Conversation
WalkthroughThis change introduces new methods across several interfaces and implementations to compute Git status by comparing file system and database resource maps. It also exposes and standardizes a method for populating modified entities in Git status. Additional methods are added to retrieve branch tracking status, and relevant method signatures are updated for broader accessibility. Observability instrumentation was added to Git resource map construction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitFSServiceCEImpl
participant CommonGitFileUtilsCE
participant FileUtilsCEImpl
participant FSGitHandlerCEImpl
Client->>GitFSServiceCEImpl: computeGitStatus(dto, artifactJson)
GitFSServiceCEImpl->>CommonGitFileUtilsCE: computeGitStatus(path, artifactJson, branch)
CommonGitFileUtilsCE->>FileUtilsCEImpl: computeGitStatus(path, resourceMap, branch, flag)
FileUtilsCEImpl->>FSGitHandlerCEImpl: populateModifiedEntities(statusDTO)
FileUtilsCEImpl-->>CommonGitFileUtilsCE: Mono<GitStatusDTO>
CommonGitFileUtilsCE-->>GitFSServiceCEImpl: Mono<GitStatusDTO>
GitFSServiceCEImpl-->>Client: Mono<GitStatusDTO>
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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
|
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15529244864. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1)
294-350: Well-implemented Git status computation with one error handling improvement needed.The method correctly implements the Git status logic by comparing database and file system resource maps. The reactive flow and parallel stream usage are appropriate for performance. However, consider improving error handling in the modified files detection.
Consider improving error handling in lines 333-339:
.filter(filePath -> { Object fileInDB = filePathToObjectsFromDB.get(filePath); Object fileInFS = filePathObjectsMapFromFS.get(filePath); try { return fileOperations.hasFileChanged(fileInDB, fileInFS); } catch (IOException e) { - log.error("Error while checking if file has changed", e); - return false; + log.error("Error while checking if file has changed for path: {}", filePath, e); + // Consider whether to include or exclude files with comparison errors + // Current approach may miss actual changes due to I/O issues + return true; // or throw a runtime exception to fail fast } })The current approach of returning
falseon IOException might silently ignore actual file changes when there are I/O issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(2 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (13)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (1)
163-163: LGTM: Clean interface addition.The new method signature follows established patterns and integrates well with the existing Git status functionality.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (2)
3-3: LGTM: Necessary import addition.The GitStatusDTO import is required for the new computeGitStatus method.
46-48: LGTM: Well-designed method signature.The computeGitStatus method has appropriate parameters for in-memory Git status computation and follows the reactive programming patterns used throughout the codebase.
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)
938-938: LGTM: Visibility change required for interface compliance.The method visibility change from protected to public is necessary to implement the FSGitHandler interface contract properly.
1568-1568:Details
✅ Verification successful
Verify that callers pass suffixes, not full paths.
The change from
repoPath.toFile()tocreateRepoPath(repoPath).toFile()suggests the method now expects a path suffix rather than a full repository path. This could break existing callers if they were passing absolute paths.
🏁 Script executed:
#!/bin/bash # Description: Find all callers of getBranchTrackingStatus to verify they pass suffixes # Expected: Callers should be passing path suffixes, not absolute paths # Search for method calls to getBranchTrackingStatus ast-grep --pattern 'getBranchTrackingStatus($_, $_)' # Also search for any usage patterns rg -A 3 -B 1 "getBranchTrackingStatus"Length of output: 6073
No breaking change: callers already pass suffixes
Verified that the only caller ofFSGitHandlerCEImpl.getBranchTrackingStatusinGitFSServiceCEImplusesgitArtifactHelper.getRepoSuffixPath(...)(a suffix), and no other call sites supply absolute paths. No changes needed.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
4-4: Import addition looks good.The new import for
GitStatusDTOis necessary for thecomputeGitStatusmethod return type.
198-212: Well-structured implementation following established patterns.The method correctly:
- Creates
GitResourceMapfrom artifact exchange JSON- Checks feature flag for reset optimization
- Delegates to
fileUtils.computeGitStatuswith proper reactive scheduling- Handles exceptions appropriately by returning
Mono.errorThe implementation is consistent with other methods in the class and follows the reactive programming paradigm correctly.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)
16-16: Import addition is appropriate.The
BranchTrackingStatusimport is required for the newgetBranchTrackingStatusmethod.
89-92: Interface method declarations are well-defined.Both new methods follow consistent naming conventions and have appropriate parameter types:
computeGitStatustakes transformation DTO and artifact exchange JSONgetBranchTrackingStatustakes transformation DTOReturn types are properly typed with reactive
Monowrappers.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (3)
45-45: Import addition is correct.The
BranchTrackingStatusimport supports the newgetBranchTrackingStatusmethod implementation.
753-764: Clean implementation following established patterns.The method properly:
- Extracts necessary parameters from the transformation DTO
- Resolves the appropriate
GitArtifactHelper- Computes the repository suffix path
- Delegates to
commonGitFileUtils.computeGitStatusThe implementation is consistent with other methods in the class and maintains the reactive flow.
766-776: Consistent implementation with clear delegation.The method follows the same pattern as other methods in the class:
- Parameter extraction from DTO
- Artifact helper resolution
- Path computation
- Delegation to
fsGitHandler.getBranchTrackingStatusThe implementation maintains consistency with the class's architectural patterns.
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1)
3-3: LGTM: Import addition is appropriate.The new import for
GitStatusDTOis correctly added to support the new method.
|
Deploy-Preview-URL: https://ce-40891.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15529395707. |
|
Deploy-Preview-URL: https://ce-40891.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15532274379. |
|
Deploy-Preview-URL: https://ce-40891.dp.appsmith.com |
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15601820881. |
|
Deploy-Preview-URL: https://ce-40891.dp.appsmith.com |
2 participants
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.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15607977360
Commit: b50460f
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 12 Jun 2025 11:13:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements