feat: add git route aspect for branch handling #41097
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8ac95f8 to
aa8c365
Compare
aa8c365 to
cb0a541
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (1)
38-50: Inconsistent deprecation strategy across Redis methods.Not all Redis-interacting methods are deprecated or have in-memory checks. Methods like
addFileLock(String key, String gitCommand)at line 38,setAutoCommitProgress, andgetAutoCommitProgressstill interact with Redis even when Git is in-memory. This inconsistency could cause issues.Consider applying the same deprecation and in-memory bypass pattern to all Redis-related methods:
public Mono<Boolean> addFileLock(String key, String gitCommand) { + if (gitServiceConfig.isGitInMemory()) { + return Mono.just(true); + } String command = hasText(gitCommand) ? gitCommand : REDIS_FILE_LOCK_VALUE;public Mono<Boolean> setAutoCommitProgress(String defaultApplicationId, Integer progress) { + if (gitServiceConfig.isGitInMemory()) { + return Mono.just(true); + } String key = String.format(AUTO_COMMIT_PROGRESS_KEY_FORMAT, defaultApplicationId);public Mono<Integer> getAutoCommitProgress(String defaultApplicationId) { + if (gitServiceConfig.isGitInMemory()) { + return Mono.just(0); + } String key = String.format(AUTO_COMMIT_PROGRESS_KEY_FORMAT, defaultApplicationId);Also applies to: 52-63, 65-71, 73-79, 81-93, 105-112, 114-127, 134-163
🧹 Nitpick comments (6)
app/server/appsmith-git/src/main/resources/application.properties (1)
2-3: Hard-coding the git root loses the previous “just set an env var” flexibilityCommenting out the
${APPSMITH_GIT_ROOT:}placeholder means developers must now override the value via higher-precedence property sources instead of a simple env var substitution inside the file. If that loss of convenience is accidental, retain the placeholder and keep/dev/shm/git-storageas the default:-# appsmith.git.root = ${APPSMITH_GIT_ROOT:} -appsmith.git.root = /dev/shm/git-storage +# Prefer env-var, otherwise fall back to the new in-memory default +appsmith.git.root = ${APPSMITH_GIT_ROOT:/dev/shm/git-storage}deploy/docker/fs/opt/appsmith/run-with-env.sh (1)
38-39: Usetlogfor consistency instead of rawechoAll other log messages in this script go through the
tloghelper; using plainecho >&2makes grepping logs harder.-echo "APPSMITH_GIT_ROOT: ${APPSMITH_GIT_ROOT}" >&2 +tlog "APPSMITH_GIT_ROOT: ${APPSMITH_GIT_ROOT}"app/server/appsmith-server/pom.xml (1)
448-459: Prefer the<release>flag over separate<source>/<target>Using the
--releaseswitch guarantees the correct standard-library APIs for the chosen runtime and avoids mismatch between compiler and linker targets.- <compilerArgs> - <arg>-parameters</arg> - </compilerArgs> - <source>17</source> - <target>17</target> + <compilerArgs> + <arg>-parameters</arg> + </compilerArgs> + <release>17</release>Dockerfile (2)
14-19: Shrink image size & layer count by collapsing APT commandsTwo consecutive
apt-get updatecalls create an extra layer and slow the build. Combine them and add--no-install-recommendsto reduce image bloat.-RUN apt-get update && \ - apt-get install -y software-properties-common && \ - add-apt-repository -y ppa:git-core/ppa && \ - apt-get update && \ - apt-get install -y git tar zstd openssh-client && \ +RUN apt-get update && \ + apt-get install -y --no-install-recommends software-properties-common && \ + add-apt-repository -y ppa:git-core/ppa && \ + apt-get update && \ + apt-get install -y --no-install-recommends git tar zstd openssh-client && \ apt-get clean && \ rm -rf /var/lib/apt/lists/*
40-42: Directory already created at runtime byrun-with-env.sh
run-with-env.shalso invokesmkdir -pv "$APPSMITH_GIT_ROOT"; duplicating the step in the image may be harmless but is redundant and can be dropped to keep the Dockerfile lean.app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java (1)
17-19: Consider making the in-memory detection more robust.The hardcoded "/dev/shm/" path check is Linux-specific and may not work across different operating systems. Consider using a more explicit configuration property or system property to determine in-memory mode.
- public Boolean isGitInMemory() { - return gitRootPath.startsWith("/dev/shm/"); - } + @Value("${appsmith.git.inmemory:false}") + private boolean gitInMemory; + + public Boolean isGitInMemory() { + return gitInMemory || gitRootPath.startsWith("/dev/shm/"); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
Dockerfile(2 hunks)app/server/.gitignore(1 hunks)app/server/appsmith-git/pom.xml(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/dto/BashFunctionResult.java(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(3 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java(1 hunks)app/server/appsmith-git/src/main/resources/application.properties(1 hunks)app/server/appsmith-git/src/main/resources/git.sh(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(1 hunks)app/server/appsmith-server/pom.xml(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/GitRoute.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifact.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.md(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java(22 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java(6 hunks)app/server/appsmith-server/src/main/resources/application-ce.properties(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java(0 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java(0 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(0 hunks)app/server/appsmith-server/src/test/resources/application-test.properties(1 hunks)deploy/docker/fs/opt/appsmith/run-with-env.sh(1 hunks)
💤 Files with no reviewable changes (3)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/index.mdc:0-0
Timestamp: 2025-06-23T12:22:10.042Z
Learning: Pull request titles in the Appsmith codebase should follow the Conventional Commits specification, using the format 'type(scope): description' with valid types such as feat, fix, docs, style, refactor, perf, test, build, ci, chore, and revert.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/pullRequest.ts:6-10
Timestamp: 2024-12-05T10:58:36.272Z
Learning: Error handling is not required for the `pullRequest` function in `app/client/src/git/requests/pullRequest.ts`.
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/requests/fetchBranchesRequest.ts:11-11
Timestamp: 2024-12-11T08:24:27.585Z
Learning: In `app/client/src/git/requests/fetchBranchesRequest.ts`, the default parameter handling for `params` works as intended. The check `if (params.pruneBranches)` is sufficient.
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/README.md:0-0
Timestamp: 2025-06-30T09:25:38.530Z
Learning: Pull request titles must follow semantic conventions as described in 'semantic-pr.md'
Learnt from: brayn003
PR: appsmithorg/appsmith#40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/checkoutBranchRequest.ts:9-17
Timestamp: 2024-12-05T11:00:14.171Z
Learning: Error handling is not required for the `checkoutBranchRequest` function in `app/client/src/git/requests/checkoutBranchRequest.ts`.
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/index.md:0-0
Timestamp: 2025-06-30T09:25:58.488Z
Learning: Validate that PR titles follow the Conventional Commits specification
app/server/.gitignore (4)
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-06-12T11:13:15.679Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: brayn003
PR: appsmithorg/appsmith#40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-06-12T11:16:26.661Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Dockerfile (3)
Learnt from: sharat87
PR: appsmithorg/appsmith#34317
File: deploy/docker/base.dockerfile:30-30
Timestamp: 2024-10-08T15:32:34.115Z
Learning: sharat87 prefers not to pin specific versions of software packages in the Dockerfile for the project, opting instead to automatically use the latest major versions.
Learnt from: sharat87
PR: appsmithorg/appsmith#34317
File: deploy/docker/base.dockerfile:30-30
Timestamp: 2024-06-20T02:30:21.454Z
Learning: sharat87 prefers not to pin specific versions of software packages in the Dockerfile for the project, opting instead to automatically use the latest major versions.
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/restore.ts:206-216
Timestamp: 2024-11-28T07:34:41.567Z
Learning: In `restore.ts`, within the `restoreDockerEnvFile` function, we intentionally make only minimal changes to the `docker.env` file and maintain a predictable order when adding environment variables.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java (5)
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
app/server/appsmith-git/src/main/resources/git.sh (6)
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:52-58
Timestamp: 2025-01-13T15:15:05.763Z
Learning: The Git sync helper functions (_.gitSync.CommitAndPush, _.gitSync.MergeToMaster) in Cypress tests already include built-in assertions, so additional explicit assertions after these calls are not needed.
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
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#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/generateSSHKeyRequest.ts:9-18
Timestamp: 2024-12-05T11:06:06.127Z
Learning: In the TypeScript file `app/client/src/git/requests/generateSSHKeyRequest.ts`, the function `generateSSHKeyRequest` does not require error handling and input validation.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (12)
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
Learnt from: brayn003
PR: appsmithorg/appsmith#38171
File: app/client/src/git/components/DefaultBranch/DefaultBranchCE.tsx:1-14
Timestamp: 2024-12-15T17:45:48.303Z
Learning: In `app/client/src/git/components/DefaultBranch/DefaultBranchCE.tsx`, the feature flag check is performed at a higher level, so it's acceptable to have `isGitProtectedFeatureLicensed={false}` in this component.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `updateJsLibsInContext` method in the `ApplicationJsLibServiceCEImpl` class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJsLibsInContext` method in the `ApplicationJsLibServiceCEImpl` class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: abhvsn
PR: appsmithorg/appsmith#39171
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java:42-44
Timestamp: 2025-02-24T05:59:14.021Z
Learning: The reactive transition for helper methods consuming @FeatureFlagged annotations will be handled in a separate PR to maintain focused changes and proper separation of concerns.
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
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`.
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#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:102-102
Timestamp: 2025-01-13T15:14:12.806Z
Learning: In Cypress Git sync tests, explicit assertions after _.gitSync.SwitchGitBranch() are not required as the helper method likely handles the verification internally.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (4)
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: brayn003
PR: appsmithorg/appsmith#38563
File: app/client/src/git/components/QuickActions/index.tsx:34-34
Timestamp: 2025-01-09T15:17:04.536Z
Learning: In Git-related components, `isStatusClean` with a default value of `true` is used to determine the initial loading state, rather than indicating the presence of uncommitted changes.
app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java (3)
Learnt from: brayn003
PR: appsmithorg/appsmith#38563
File: app/client/src/git/components/QuickActions/index.tsx:34-34
Timestamp: 2025-01-09T15:17:04.536Z
Learning: In Git-related components, `isStatusClean` with a default value of `true` is used to determine the initial loading state, rather than indicating the presence of uncommitted changes.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
app/server/appsmith-server/src/test/resources/application-test.properties (13)
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-06-12T11:16:26.661Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#37292
File: app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java:0-0
Timestamp: 2024-11-09T11:37:30.846Z
Learning: In test files within the Appsmith project, hardcoded strings are acceptable and do not need to be extracted into constants.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-06-12T11:13:15.679Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts:7-7
Timestamp: 2024-11-28T07:22:10.857Z
Learning: In the `DiskSpaceLink` class in `app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts`, it's acceptable to hardcode the backup path to `/appsmith-stacks`.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: sharat87
PR: appsmithorg/appsmith#37663
File: app/client/packages/rts/src/ctl/backup.ts:264-267
Timestamp: 2024-11-23T15:15:51.853Z
Learning: In `app/client/packages/rts/src/ctl/backup.ts` (TypeScript, NodeJS), it is acceptable to pass `appsmithMongoURI` directly to the `mongodump` command without masking the connection string, and exposing the connection string in command execution is acceptable in this context.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java (5)
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
app/server/appsmith-server/src/main/resources/application-ce.properties (8)
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-06-12T11:16:26.661Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36458
File: deploy/docker/fs/opt/appsmith/baseline-ce/plugin.jsonl:17-18
Timestamp: 2024-09-20T14:55:06.929Z
Learning: In the product, icon URLs are referred to using 'https://s3.us-east-2.amazonaws.com/assets.appsmith.com/...'.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36458
File: deploy/docker/fs/opt/appsmith/baseline-ce/plugin.jsonl:17-18
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the product, icon URLs are referred to using 'https://s3.us-east-2.amazonaws.com/assets.appsmith.com/...'.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
deploy/docker/fs/opt/appsmith/run-with-env.sh (11)
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-06-12T11:13:15.679Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-06-12T11:16:26.661Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/restore.ts:206-216
Timestamp: 2024-11-28T07:34:41.567Z
Learning: In `restore.ts`, within the `restoreDockerEnvFile` function, we intentionally make only minimal changes to the `docker.env` file and maintain a predictable order when adding environment variables.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/fs/opt/appsmith/pg-utils.sh:125-132
Timestamp: 2024-10-25T08:31:07.651Z
Learning: In `deploy/docker/fs/opt/appsmith/pg-utils.sh`, the `POSTGRES_ADMIN_USER` (`postgres`) uses trust-based authentication in the local environment, so password handling changes for local setup may not be needed.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37326
File: deploy/docker/fs/opt/appsmith/utils/bin/export_db.js:11-17
Timestamp: 2024-11-13T09:41:48.119Z
Learning: In the `export_db.js` script located at `deploy/docker/fs/opt/appsmith/utils/bin/export_db.js`, it's acceptable to use simple string-based detection (e.g., `dbUrl.startsWith('mongodb')`) for determining the database type because the database URL is restrictive, and misconfiguration will cause the server to fail to start up. Therefore, using a URL parsing library for database URL detection is unnecessary in this context.
app/server/appsmith-git/src/main/resources/application.properties (12)
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-06-12T11:16:26.661Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/utils/bin/index.js:18-18
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The configuration file `/appsmith-stacks/configuration/docker.env` is ensured to exist at runtime within the Docker container, as clarified by sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: sharat87
PR: appsmithorg/appsmith#28081
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:7-7
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `TMP` environment variable is set in the Docker image for the Appsmith project, as clarified by the user sharat87.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#34206
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:84-95
Timestamp: 2024-06-12T11:13:15.679Z
Learning: The environment files `/appsmith-stacks/configuration/docker.env` and `$TMP/pre-define.env` are generated at runtime, which is why they may not be found in static checks but will exist during actual execution.
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts:7-7
Timestamp: 2024-11-28T07:22:10.857Z
Learning: In the `DiskSpaceLink` class in `app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts`, it's acceptable to hardcode the backup path to `/appsmith-stacks`.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/fs/opt/appsmith/pg-utils.sh:125-132
Timestamp: 2024-10-25T08:31:07.651Z
Learning: In `deploy/docker/fs/opt/appsmith/pg-utils.sh`, the `POSTGRES_ADMIN_USER` (`postgres`) uses trust-based authentication in the local environment, so password handling changes for local setup may not be needed.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (11)
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/checkoutBranchRequest.ts:9-17
Timestamp: 2024-12-05T11:00:14.171Z
Learning: Error handling is not required for the `checkoutBranchRequest` function in `app/client/src/git/requests/checkoutBranchRequest.ts`.
Learnt from: brayn003
PR: appsmithorg/appsmith#38563
File: app/client/src/git/components/QuickActions/index.tsx:34-34
Timestamp: 2025-01-09T15:17:04.536Z
Learning: In Git-related components, `isStatusClean` with a default value of `true` is used to determine the initial loading state, rather than indicating the presence of uncommitted changes.
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`.
Learnt from: brayn003
PR: appsmithorg/appsmith#38155
File: app/client/src/git/components/ProtectedBranches/index.tsx:18-27
Timestamp: 2024-12-13T22:08:46.336Z
Learning: The `ProtectedBranchesView` component in `app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx` already has a TypeScript interface for its props.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchSSHKeyRequest.ts:5-9
Timestamp: 2024-12-05T10:56:13.739Z
Learning: In `app/client/src/git/requests/fetchSSHKeyRequest.ts`, the `fetchSSHKeyRequest` function does not require explicit error handling for `baseApplicationId`; the function should throw an error naturally if `baseApplicationId` is missing or invalid.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/generateSSHKeyRequest.ts:9-18
Timestamp: 2024-12-05T11:06:06.127Z
Learning: In the TypeScript file `app/client/src/git/requests/generateSSHKeyRequest.ts`, the function `generateSSHKeyRequest` does not require error handling and input validation.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/generateSSHKeyRequest.types.ts:6-11
Timestamp: 2024-12-05T11:02:42.148Z
Learning: In the `GenerateSSHKeyResponse` interface in `app/client/src/git/requests/generateSSHKeyRequest.types.ts`, both `isRegeneratedKey` and `regeneratedKey` are intentionally included and serve the same concept.
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.md (1)
Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/task-list.mdc:0-0
Timestamp: 2025-06-23T12:22:13.688Z
Learning: Implementation details such as architecture decisions, data flow, technical components, and environment configuration should be documented in the task list markdown file.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java (3)
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#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.
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.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (3)
Learnt from: brayn003
PR: appsmithorg/appsmith#38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
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.
🧬 Code Graph Analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifact.java (1)
Component(6-12)
🪛 Shellcheck (0.10.0)
app/server/appsmith-git/src/main/resources/git.sh
[warning] 21-21: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 LanguageTool
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.md
[grammar] ~1-~1: Use proper spacing conventions.
Context: ## GitRoute FSM Execution Flow mermaid flowchart TD START([START]) --> ARTIFACT ARTIFACT -->|SUCCESS| PARENT ARTIFACT -->|FAIL| RESULT PARENT -->|SUCCESS| GIT_META PARENT -->|FAIL| RESULT GIT_META -->|SUCCESS| REPO_KEY GIT_META -->|FAIL| RESULT REPO_KEY -->|SUCCESS| LOCK_KEY REPO_KEY -->|FAIL| RESULT LOCK_KEY -->|SUCCESS| LOCK LOCK_KEY -->|FAIL| RESULT LOCK -->|SUCCESS| GIT_PROFILE LOCK -->|FAIL| RESULT GIT_PROFILE -->|SUCCESS| GIT_AUTH GIT_PROFILE -->|FAIL| UNLOCK GIT_AUTH -->|SUCCESS| GIT_KEY GIT_AUTH -->|FAIL| UNLOCK GIT_KEY -->|SUCCESS| REPO_PATH GIT_KEY -->|FAIL| UNLOCK REPO_PATH -->|SUCCESS| DOWNLOAD REPO_PATH -->|FAIL| UNLOCK DOWNLOAD -->|SUCCESS| EXECUTE DOWNLOAD -->|FAIL| UNLOCK EXECUTE -->|SUCCESS or FAIL| UPLOAD UPLOAD -->|SUCCESS or FAIL| UNLOCK UNLOCK -->|SUCCESS or FAIL| RESULT RESULT --> DONE([DONE])
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (22)
app/server/.gitignore (1)
15-15: LGTM! Good practice improvement.Broadening the pattern from
*.envto*.env*correctly handles environment files with suffixes (e.g.,.env.local,.env.production), preventing accidental commits of sensitive configuration data.app/server/appsmith-git/pom.xml (1)
69-77: No vulnerabilities found for commons-exec 1.3GitHub’s Maven advisory database reports no known security issues for org.apache.commons:commons-exec@1.3. The reactor-core dependency inherits its version from the parent POM, so it aligns with your project’s standards.
• commons-exec 1.3 – no advisories found
• reactor-core – version inheritance confirmedapp/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (1)
163-171: Well-designed API addition.The new overloaded
mergeBranchmethod provides a cleaner interface for cases where working directory changes aren't a concern. Good documentation of the precondition that the destination branch must be checked out.app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java (1)
135-140: Comprehensive error code coverage for Git route handling.The new error codes follow established patterns and provide good coverage for the Git route framework failure scenarios. The sequential numbering and descriptive names align with existing conventions.
app/server/appsmith-server/src/main/resources/application-ce.properties (1)
69-69: Good configuration design with sensible fallback.The new
appsmith.redis.git.urlproperty allows dedicated Redis configuration for Git operations while falling back to the existing Redis URL. This supports the enhanced Git caching architecture.app/server/appsmith-server/src/test/resources/application-test.properties (1)
4-5: /dev/shm may not exist on every OS running the test-suite
/dev/shmis Linux-specific; macOS and many CI runners on other kernels will silently fall back to disk or throw “No such file or directory”, breaking tests that rely on the path existing.
Please either (a) parameterise the path through an env var with a sane default, or (b) create the directory in the test bootstrap before it is accessed.- appsmith.git.root = /dev/shm/git-storage + # Defaults to in-memory on Linux; falls back to tmp on other OSes. + appsmith.git.root = ${APPSMITH_GIT_ROOT:/tmp/git-storage}app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifact.java (1)
6-12: LGTM - Standard Spring component implementation.The class follows proper Spring component patterns with clear dependency injection. The constructor correctly delegates to the superclass.
app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/GitRoute.java (1)
11-18: Well-structured annotation for Git route processing.The annotation is properly configured with appropriate targets, runtime retention, and documentation. The required elements
fieldNameandartifactTypealign well with the Git route handling use case.app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java (1)
11-12: Ensureappsmith.git.rootis set in every deployment environmentWe ran a search across
.properties,.ymland.yamlfiles and foundappsmith.git.rootonly in:
app/server/appsmith-git/src/main/resources/application.propertiesapp/server/appsmith-server/src/test/resources/application-test.propertiesIt’s not present in any Docker Compose, Kubernetes manifests or
.envfiles. Since the default value was removed, missing this configuration will cause startup failures.• Add
appsmith.git.root(or the equivalentAPPSMITH_GIT_ROOTenv var) to:
– your Docker-generateddocker.envor compose files
– any Helm/values.yaml or Kubernetes manifests
– local.envor other profiles used in non-Docker dev setups• Document this new requirement in the README or deploy docs so that all team members and CI/CD pipelines include it.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java (1)
22-46: LGTM - Constructor properly updated for GitServiceConfig injection.The constructor changes correctly add the GitServiceConfig parameter and pass it to the superclass, following standard dependency injection patterns.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java (1)
22-46: LGTM - Consistent constructor update for GitServiceConfig injection.The constructor changes are consistent with the pattern used in
GitFSServiceImpland properly integrate the new GitServiceConfig dependency.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.md (1)
1-44: Excellent FSM documentation for Git route processing.The Mermaid diagram clearly illustrates the finite state machine execution flow with proper error handling paths. This documentation will be valuable for understanding the complex Git route orchestration workflow.
app/server/appsmith-git/src/main/java/com/appsmith/git/dto/BashFunctionResult.java (1)
1-14: Clean and well-designed DTO for bash function results.The class appropriately uses Lombok annotations and provides a clear structure for capturing bash execution output, exit codes, and error messages.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
83-83: Dependency injection properly added for Git service configuration.The
GitServiceConfiginjection enables conditional Git operation handling based on storage type.
704-707: Smart optimization for in-memory Git operations.The conditional logic efficiently bypasses feature flag checks and uses a simplified merge operation when Git is configured for in-memory storage, improving performance for this use case.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (2)
9-9: Import added for the new GitRoute annotation.Clean addition of the necessary import to support the aspect-driven Git route handling.
63-63: Consistent @gitroute annotation application across all Git operations.All controller methods are properly annotated with
@GitRoutespecifying the correct field names and artifact type. This enables the aspect-driven Git route orchestration while maintaining existing API behavior.Also applies to: 72-72, 85-85, 97-97, 113-113, 123-123, 133-133, 143-143, 155-155, 167-167, 182-182, 197-197, 210-210, 220-220, 231-231, 240-240, 249-249, 260-260, 269-269, 282-282, 291-291
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java (2)
11-17: Proper component structure with dependency injection.Clean abstract class design with constructor-based dependency injection following Spring best practices.
19-27: Well-implemented reactive artifact resolver with proper error handling.The switch expression cleanly handles the APPLICATION case with proper reactive error handling for missing artifacts. The default case appropriately returns an error for unsupported artifact types, allowing for future extensibility.
app/server/appsmith-git/src/main/resources/git.sh (1)
28-28: Verify shallow clone compatibility with mergesShallow cloning (
--depth=1) can breakgit mergeif required history is missing. Ensure yourgit_merge_branchroutine either fetches full history or handles unshallowing before merging.• File: app/server/appsmith-git/src/main/resources/git.sh
– Line 28:git fetch origin --depth=1
• Reviewgit_merge_branchto confirm it runsgit fetch --unshallow(or uses a deeper clone) before performing merges.Please double-check this flow to avoid “insufficient history” errors during merge operations.
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java (1)
1023-1062: New error constants look good.The Git route error constants follow the established pattern and use appropriate HTTP status codes.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (1)
22-139: Consistent deprecation implementation.The entire class is properly deprecated with all methods consistently checking for in-memory mode.
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
Show resolved
Hide resolved
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
Show resolved
Hide resolved
app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java
Show resolved
Hide resolved
app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java
Show resolved
Hide resolved
app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/16168390185. |
|
Deploy-Preview-URL: https://ce-41097.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
WalkthroughThis update introduces a comprehensive Git route orchestration mechanism using a finite state machine (FSM) aspect, shell-based Git operations, and in-memory Git storage support. It adds new annotations, error codes, Bash scripts, and supporting classes for managing Git operations, concurrency, and artifact retrieval. Several test cases are removed, and configuration files are updated for in-memory Git storage paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant GitRouteAspect
participant GitRouteArtifact
participant BashService
participant Redis
Client->>Controller: HTTP request (Git operation)
Controller->>GitRouteAspect: Method intercepted (@GitRoute)
GitRouteAspect->>GitRouteArtifact: Retrieve artifact by type/id
GitRouteAspect->>Redis: Acquire lock (if not in-memory)
GitRouteAspect->>BashService: Download repo via git.sh
GitRouteAspect->>Controller: Proceed with original method
Controller-->>GitRouteAspect: Result
GitRouteAspect->>BashService: Upload repo via git.sh
GitRouteAspect->>Redis: Release lock (if not in-memory)
GitRouteAspect-->>Client: Final result
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)
116-116: Use dependency injection for BashService instead of direct instantiationThe direct instantiation of
BashServicebreaks consistency with other dependencies that use constructor injection. This impacts testability and Spring's lifecycle management.Consider adding
BashServiceas a constructor parameter and ensuring it's properly annotated as a Spring component.
1113-1119: Add input validation and error handling to mergeBranch methodThe method delegates to bash script without validating inputs, which could lead to security issues. Consider adding branch name validation and proper error handling.
Add validation for branch names to ensure they contain only allowed characters and wrap the bash service call with error handling to provide meaningful exceptions.
app/server/appsmith-git/src/main/resources/git.sh (2)
21-21: Declare and assign separately to avoid masking return valuesStatic analysis correctly identifies that combining declaration and assignment can mask the return value of
mktemp.- local temp_private_key=$(mktemp /dev/shm/tmp.XXXXXX) + local temp_private_key + temp_private_key=$(mktemp /dev/shm/tmp.XXXXXX)
72-77: Don't mask checkout errorsUsing
|| trueon line 75 masks legitimate checkout failures. Consider logging errors instead of silently ignoring them.for remote in $(git -C "$target_folder" branch -r | grep -vE 'origin/HEAD'); do branch=${remote#origin/} if ! git -C "$target_folder" show-ref --quiet "refs/heads/$branch"; then - git -C "$target_folder" checkout -b "$branch" "$remote" || true + if ! git -C "$target_folder" checkout -b "$branch" "$remote" 2>/dev/null; then + echo "Warning: Failed to checkout branch $branch" >&2 + fi fi doneapp/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java (4)
20-21: Add Spring stereotype annotationThe class needs
@Componentor@Serviceannotation to enable dependency injection in Spring context.+import org.springframework.stereotype.Component; + @Slf4j +@Component public class BashService {
79-91: Critical: Command injection vulnerabilityThe method doesn't escape shell special characters in arguments, allowing command injection. Arguments containing characters like
;,|,&,$()etc. could execute arbitrary commands.private String buildFullCommand(String scriptContent, String functionName, String... args) { String variableAssignments = IntStream.range(0, args.length) - .mapToObj(i -> String.format("arg%d=\"%s\"", i + 1, args[i])) + .mapToObj(i -> String.format("arg%d=%s", i + 1, escapeShellArg(args[i]))) .collect(Collectors.joining("\n"));Add a method to properly escape shell arguments:
private String escapeShellArg(String arg) { if (arg == null) return "''"; return "'" + arg.replace("'", "'\"'\"'") + "'"; }
41-75: Use try-with-resources for stream managementStreams should be managed with try-with-resources to ensure proper cleanup even if exceptions occur.
- ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); - ByteArrayOutputStream errorStream = new ByteArrayOutputStream(); - ByteArrayInputStream inputStream = new ByteArrayInputStream(fullScript.getBytes(StandardCharsets.UTF_8)); + try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + ByteArrayOutputStream errorStream = new ByteArrayOutputStream(); + ByteArrayInputStream inputStream = new ByteArrayInputStream(fullScript.getBytes(StandardCharsets.UTF_8))) { // ... existing code ... - outputStream.close(); - errorStream.close(); - inputStream.close(); - - return new BashFunctionResult(output, exitCode, error); + return new BashFunctionResult(output, exitCode, error); + }
104-111: Fix parameter order in error detailsThe formatted string has incorrect parameter order - stdout and stderr are swapped.
private String buildErrorDetails(String output, String error, String exceptionError, Integer exitCode) { return "EXITCODE: %s\nEXCEPTION: %s\nSTDERR: %s\nSTDOUT: %s" .formatted( fallbackIfBlank(exitCode, "(empty)"), - fallbackIfBlank(output, "(empty)"), - fallbackIfBlank(error, "(empty)"), - fallbackIfBlank(exceptionError, "(none)")); + fallbackIfBlank(exceptionError, "(none)"), + fallbackIfBlank(error, "(empty)"), + fallbackIfBlank(output, "(empty)")); }app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java (4)
57-57: Use dependency injection for BashService.Direct instantiation breaks the dependency injection pattern and makes testing difficult.
- private final BashService bashService = new BashService(); + private final BashService bashService;Add it to the constructor parameters and let Spring inject it.
164-167: Fix inverted in-memory check logic.The condition bypasses the FSM when Git is not in memory, but it should bypass when Git IS in memory (FS mode).
- // If Git is not in memory, we can just execute the join point - if (!gitServiceConfig.isGitInMemory()) { + // If Git is in memory, bypass the FSM and directly execute the join point + if (gitServiceConfig.isGitInMemory()) { return execute(ctx); }
268-276: Improve error handling for SSH key processing.The generic exception catch could hide specific issues with key processing.
private Mono<?> gitKey(Context ctx) { try { return Mono.just(processPrivateKey( ctx.getGitAuth().getPrivateKey(), ctx.getGitAuth().getPublicKey())); } catch (Exception e) { + log.error("Failed to process Git SSH key", e); return Mono.error(new AppsmithException( AppsmithError.INVALID_GIT_CONFIGURATION, "Failed to process private key: " + e.getMessage())); } }
342-350: Avoid reflection for setting context fields.Using reflection to set fields is fragile and can fail at runtime if field names change.
Consider adding setter methods to the Context class and using a switch statement:
- private static void setContextField(Context ctx, String fieldName, Object value) { - try { - var field = ctx.getClass().getDeclaredField(fieldName); - field.setAccessible(true); - field.set(ctx, value); - } catch (Exception e) { - throw new RuntimeException("Failed to set field " + fieldName, e); - } - } + private static void setContextField(Context ctx, String fieldName, Object value) { + switch (fieldName) { + case "artifact" -> ctx.setArtifact((Artifact) value); + case "parent" -> ctx.setParent((Artifact) value); + case "gitMeta" -> ctx.setGitMeta((GitArtifactMetadata) value); + case "repoKey" -> ctx.setRepoKey((String) value); + case "lockKey" -> ctx.setLockKey((String) value); + case "lock" -> ctx.setLock((Boolean) value); + case "gitProfile" -> ctx.setGitProfile((GitProfile) value); + case "gitAuth" -> ctx.setGitAuth((GitAuth) value); + case "gitKey" -> ctx.setGitKey((String) value); + case "repoPath" -> ctx.setRepoPath((String) value); + case "download" -> ctx.setDownload(value); + case "execute" -> ctx.setExecute(value); + case "upload" -> ctx.setUpload(value); + case "unlock" -> ctx.setUnlock((Boolean) value); + case "result" -> ctx.setResult(value); + default -> throw new IllegalArgumentException("Unknown field: " + fieldName); + } + }
🧹 Nitpick comments (9)
app/server/.gitignore (1)
15-15: Check for unintended exclusion of template env files
*.env*will also ignore handy reference files like.env.exampleor.env.sample. If the project relies on those being version-controlled, add negation rules:*.env* +!*.env.example +!*.env.samplePlease verify whether sample env files exist or are expected in the repo.
app/server/appsmith-git/src/main/resources/application.properties (1)
2-3: Configuration change to hardcoded in-memory path.The shift from environment variable configuration to hardcoded
/dev/shm/git-storageremoves deployment flexibility but aligns with the new in-memory Git storage architecture. Consider whether this hardcoded path might cause issues in different deployment environments.app/server/appsmith-server/pom.xml (1)
448-459: Prefer the<release>flag for JDK 17 compilation
source+targetworks, but Maven’s compiler plugin recommends using the single<release>element to guarantee the correct standard library APIs are linked:- <source>17</source> - <target>17</target> + <release>17</release>Also double-check that the parent‐level
maven-compiler-plugin(if any) doesn’t already configure this, otherwise you’ll end up with two plugin declarations.app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/GitRoute.java (1)
11-18: Consider sane defaults for annotation attributesRequiring both
fieldNameandartifactTypeon every usage is verbose. Provide defaults (e.g., empty string /ArtifactType.APPLICATION) so callers supply only when they deviate.- String fieldName(); - ArtifactType artifactType(); + String fieldName() default ""; + ArtifactType artifactType() default ArtifactType.APPLICATION;Dockerfile (2)
14-19: Set non-interactive frontend before apt and collapse updatesRunning
add-apt-repositoryin CI can hang withoutDEBIAN_FRONTEND=noninteractive. Also a singleapt-get updateis enough:-RUN apt-get update && \ - apt-get install -y software-properties-common && \ - add-apt-repository -y ppa:git-core/ppa && \ - apt-get update && \ +RUN DEBIAN_FRONTEND=noninteractive \ + apt-get update && \ + apt-get install -y software-properties-common && \ + add-apt-repository -y ppa:git-core/ppa && \ + apt-get update && \
40-42: Make directory creation idempotent
mkdirwill fail if/dev/shm/git-storagealready exists (e.g., layer cache reuse). Add-p:-RUN mkdir --mode 775 "/dev/shm/git-storage" +RUN mkdir -p --mode 775 "/dev/shm/git-storage"app/server/appsmith-git/src/main/java/com/appsmith/git/dto/BashFunctionResult.java (1)
7-9: Consider removing redundant constructor annotationsUsing both
@AllArgsConstructorand@RequiredArgsConstructoris redundant since@AllArgsConstructoralready provides a constructor with all fields. Consider removing@RequiredArgsConstructor.@Data @AllArgsConstructor -@RequiredArgsConstructor public class BashFunctionResult {app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
704-708: Document the behavioral difference for in-memory Git modeThe in-memory Git mode bypasses the
keepWorkingDirChangesfeature flag logic, which could lead to different merge behaviors. Consider adding a comment explaining why this different path is taken.+ // In-memory Git mode uses a simplified merge approach without working directory optimization if (gitServiceConfig.isGitInMemory()) { return fsGitHandler.mergeBranch( repoSuffix, gitMergeDTO.getSourceBranch(), gitMergeDTO.getDestinationBranch()); }app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (1)
52-63: Document the deprecation strategy and in-memory behaviorThese methods are marked
@Deprecatedbut still contain active logic for non-in-memory mode. Consider:
- Adding Javadoc explaining when these methods will be removed
- Documenting the in-memory bypass behavior
- Creating a migration plan for callers
Example:
/** * @deprecated This method bypasses Redis when Git runs in-memory mode. * Will be removed once all Git operations support in-memory mode. */ @DeprecatedAlso applies to: 65-72, 73-79, 81-93, 105-112, 114-127, 134-163
sondermanish
approved these changes
Jul 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/16343398654
Commit: f8257de
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 17 Jul 2025 12:14:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores
Tests