Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: git branches IT #38805

Merged
merged 5 commits into from
Jan 27, 2025
Merged

chore: git branches IT #38805

merged 5 commits into from
Jan 27, 2025

Conversation

sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Jan 22, 2025

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 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.Git, @tag.ImportExport"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12983176503
Commit: 9462078
Cypress dashboard.
Tags: @tag.Git, @tag.ImportExport
Spec:


Mon, 27 Jan 2025 07:09:02 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Added file format versioning for Git-related artifacts
    • Enhanced Git reference management with new methods and constants
    • Improved logging for Git operations
  • Bug Fixes

    • Refined file path matching and handling in Git operations
    • Improved error handling during artifact and metadata processing
  • Refactor

    • Centralized file and constant management across Git-related services
    • Updated method signatures to support more comprehensive Git workflows
    • Simplified code by removing hardcoded string literals
  • Tests

    • Added new integration test suite for Git branch and repository operations

@sondermanish sondermanish requested a review from a team as a code owner January 22, 2025 15:41
@sondermanish sondermanish self-assigned this Jan 22, 2025
Copy link
Contributor

coderabbitai bot commented Jan 22, 2025

Walkthrough

This pull request introduces comprehensive modifications to the Git-related file handling and operations across multiple classes in the Appsmith server. The changes focus on centralizing constants, refining method signatures, enhancing logging, and improving error handling in Git-related services. Key areas of modification include file utilities, service implementations, and integration tests, with an emphasis on standardizing file path management and Git reference handling.

Changes

File Path Change Summary
FileUtilsCEImpl.java Added static imports for constants, refined regex patterns
FileOperationsCEv2Impl.java Simplified getFileFormatVersion method return statement
FSGitHandlerCEImpl.java Updated logging levels from debug to info for key operations
GitConstantsCE.java Added README_FILE_NAME constant
ApplicationGitFileUtilsCEImpl.java Enhanced metadata handling, improved widget data processing
CentralGitServiceCEImpl.java Added new method for artifact reference generation
GitHandlingServiceCE.java Updated method signature to include additional metadata
CommonGitServiceCEImpl.java Updated import statements, replaced hardcoded strings
ArtifactJsonTransformationDTO.java Added new constructor with artifact type
GitFSServiceCEImpl.java Modified method signatures for more flexible Git operations

Sequence Diagram

sequenceDiagram
    participant Client
    participant GitService
    participant FileUtils
    participant GitHandler

    Client->>GitService: Create Git Reference
    GitService->>FileUtils: Prepare Artifact
    FileUtils-->>GitService: Artifact Prepared
    GitService->>GitHandler: Initialize Repository
    GitHandler-->>GitService: Repository Ready
    GitService->>GitHandler: Commit Changes
    GitHandler-->>GitService: Commit Successful
    GitService-->>Client: Reference Created
Loading

Possibly related PRs

Suggested Labels

git, refactoring, constants-management

Suggested Reviewers

  • nidhi-nair
  • abhvsn
  • sharat87

Poem

🌿 Git's Garden of Code

Constants bloom like springtime flowers,
Refactored paths through coding bowers,
Logs shine bright, no longer dim,
Appsmith's Git dance begins to spin!

🌈 Code grows cleaner, day by day! 🚀

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jan 22, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)

Line range hint 1095-1129: Refine error handling during commit failures

Detaching the remote repository on any commit failure might be too aggressive. Handle specific exceptions to prevent unnecessary detachment due to transient errors.

🧹 Nitpick comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (1)

269-270: Consider handling potential errors in the reactive chain.

While the implementation is correct, consider adding error handling for cases where getDefaultThemeId() fails.

 Mono<String> publishedThemeIdMono = Mono.justOrEmpty(application.getPublishedModeThemeId())
-        .switchIfEmpty(getDefaultThemeId());
+        .switchIfEmpty(getDefaultThemeId()
+            .onErrorResume(error -> {
+                log.error("Error getting default theme id", error);
+                return Mono.error(new AppsmithException(AppsmithError.THEME_NOT_FOUND));
+            }));
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)

367-376: Improve null handling with Optional.

While the current implementation handles null collections correctly, consider using Optional for a more idiomatic Java approach.

-    List<ApplicationPage> unPublishedPages = CollectionUtils.isEmpty(application.getPages())
-            ? new ArrayList<>()
-            : application.getPages();
+    List<ApplicationPage> unPublishedPages = Optional.ofNullable(application.getPages())
+            .orElseGet(ArrayList::new);

-    List<ApplicationPage> publishedPages = CollectionUtils.isEmpty(application.getPublishedPages())
-            ? new ArrayList<>()
-            : application.getPublishedPages();
+    List<ApplicationPage> publishedPages = Optional.ofNullable(application.getPublishedPages())
+            .orElseGet(ArrayList::new);
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (2)

738-754: Consider using a custom exception instead of RuntimeException

While the error handling includes proper logging, throwing a RuntimeException might be too generic. Consider:

  1. Creating a custom exception type for widget serialization errors
  2. Using AppsmithPluginException to maintain consistency with other error handling in the codebase

738-754: Consider using a custom exception for better error handling.

While the current error handling is good, consider creating a custom exception instead of using RuntimeException. This would provide better error classification and handling specific to widget deserialization failures.

-                                    throw new RuntimeException(jsonProcessingException);
+                                    throw new WidgetDeserializationException("Failed to deserialize widget: " + entry.getKey().getFilePath(), jsonProcessingException);
app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (5)

208-208: Fix typo in comment

Correct the typo in the comment: "ane" should be "an".

Apply this diff:

-            // TODO: Check why there is ane extra commit
+            // TODO: Check why there is an extra commit

209-209: Remove commented-out code

Avoid leaving commented-out code. If the assertion is still needed, uncomment it; otherwise, remove it to keep the codebase clean.


230-232: Uncomment or remove the test assertion

Decide whether the fail("Auto-commit took too long"); statement is necessary. If it is, uncomment it to ensure the test behaves as expected; if not, remove it along with the TODO comment.


248-255: Remove commented-out error message block

Commented-out error messages can clutter the code. Consider removing this block for better readability.


385-386: Complete the incomplete comment

The comment starting with "Since the status" appears incomplete. Please complete it or remove it.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)

624-633: Reduce duplication in ArtifactJsonTransformationDTO creation

Consider refactoring the creation of ArtifactJsonTransformationDTO for baseRefTransformationDTO and createRefTransformationDTO to avoid code duplication.


2992-2993: Simplify lambda expression in zipWhen

Since sourceStatusDTO isn't used, replace .zipWhen(sourceStatusDTO -> destinationBranchStatusMono) with .zipWith(destinationBranchStatusMono) for clarity.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b14669e and 00232d7.

📒 Files selected for processing (15)
  • app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (25 hunks)
  • app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java (1 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/constants/ce/GitConstantsCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (20 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java (1 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 (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (19 hunks)
✅ Files skipped from review due to trivial changes (3)
  • app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
👮 Files not reviewed due to content moderation or server errors (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java
  • app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: server-spotless / spotless-check
  • GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (20)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)

439-445: LGTM! Appropriate log level for branch operations.

The elevation of log level from debug to info for branch checkout operations improves operational visibility of significant Git operations.


615-620: LGTM! Appropriate log level for status checks.

The elevation of log level from debug to info for repository status checks improves operational visibility of important Git operations.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java (2)

Line range hint 8-8: Address or track the TODO comment.

The TODO comment suggests finding a better name for this DTO. Consider creating a tracking issue if this can't be addressed immediately.

Would you like me to suggest alternative names based on the DTO's purpose or help create a tracking issue?


38-44: LGTM! Well-structured constructor addition.

The new constructor is properly implemented, maintaining consistency with the existing pattern while adding the ability to specify the artifactType at construction time.

app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (2)

264-268: Well-documented theme publishing concept!

The comments clearly explain the rationale behind theme copying during publishing, which is crucial for maintaining code clarity.


272-274: LGTM: Robust theme publishing implementation.

The implementation correctly handles the theme publishing flow by:

  1. Using the existing published theme ID if available
  2. Falling back to default theme ID for new applications
  3. Saving the theme with appropriate application mode
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)

56-57: LGTM! Good use of static imports.

The static imports improve code readability by reducing the need to prefix the constants with their class name.

app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java (1)

170-172: LGTM! Clean refactoring.

The simplification of the return statement maintains the same logic while reducing unnecessary variable assignment.

app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (3)

Line range hint 41-95: LGTM! Well-documented implementation.

The code maintains good separation of concerns and includes comprehensive documentation explaining the theme import logic and decision tree.


Line range hint 156-167: Consider utilizing the isContextAgnostic parameter.

The new overloaded method ignores the isContextAgnostic parameter. If this parameter is meant to affect the import behavior, it should be utilized in the implementation.


75-82: Verify theme state consistency after removing published theme handling.

The simplified theme import logic now only handles unpublished themes. While this streamlines the code, we should ensure this doesn't affect theme state consistency during application publishing.

✅ Verification successful

Theme state consistency is maintained through application publishing flow

The simplified theme import is safe as theme publishing is properly handled by ApplicationPageServiceCEImpl.publish() which calls ThemeService.publishTheme(). This maintains theme state consistency during the application publishing process.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for theme publishing related code to verify consistency
rg -l "publishTheme|publishedTheme" --type java

Length of output: 1507


Script:

#!/bin/bash
# Check theme service interface and implementation
ast-grep --pattern 'interface ThemeServiceCE {
  $$$
}' app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCE.java

echo "---"

ast-grep --pattern 'class ThemeServiceCEImpl $_ {
  $$$
}' app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java

Length of output: 323


Script:

#!/bin/bash
# Get the content of ThemeImportableServiceCEImpl and search for theme publishing methods
cat app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java

echo "---"

# Search for theme publishing method calls
rg "publishTheme|publishedTheme" --type java -A 2 -B 2

Length of output: 38335

app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (4)

73-74: LGTM - New imports for file format version constants


194-197: LGTM - Added file format version to metadata

Properly adds version tracking to the metadata object.


73-74: LGTM! Imports are properly utilized.

The new imports for file format versioning constants are correctly used in the metadata handling logic.


194-197: LGTM! Good practice for version management.

Adding file format version to metadata enables proper version compatibility checks during Git operations.

app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (2)

356-358: Exception handling is appropriate

The test correctly verifies that committing to a protected branch fails as expected.


514-514: Verify the status after discarding changes

Ensure that after discarding changes, the repository status is clean. Add an assertion to verify this.

Apply this diff:

     GitStatusDTO discardableStatus = centralGitService.getStatus(checkedOutFooArtifact.getId(), artifactType, false, GitType.FILE_SYSTEM).block();

     assertThat(discardableStatus).isNotNull();
     assertThat(discardableStatus.getIsClean()).isFalse();

+    // Verify status after discarding changes
+    Artifact discardedFoo = centralGitService.discardChanges(checkedOutFooArtifact.getId(), artifactType, GitType.FILE_SYSTEM).block();
+    GitStatusDTO discardedStatus = centralGitService.getStatus(checkedOutFooArtifact.getId(), artifactType, false, GitType.FILE_SYSTEM).block();
+    assertThat(discardedStatus).isNotNull();
+    assertThat(discardedStatus.getIsClean()).isTrue();
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)

Line range hint 513-536: Implementation of remote reference checkout appears correct

The logic for handling remote reference checkout and artifact creation looks appropriate.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)

90-93: Verify update of createGitReference method signature

The method signature now includes GitArtifactMetadata baseGitData. Ensure all implementations and calls to createGitReference are updated accordingly.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)

324-325: Pass readmeFileName to getRepoSuffixPath

Including readmeFileName enhances flexibility in specifying file paths.

@@ -136,7 +139,7 @@ public class GitBranchesIT {
GitServerInitializerExtension gitServerInitializerExtension;

@Autowired
CommonGitService commonGitService;
CentralGitService centralGitService;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add @Autowired annotation for centralGitService

To ensure centralGitService is properly injected by Spring, add the @Autowired annotation.

Apply this diff:

+    @Autowired
     CentralGitService centralGitService;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
CentralGitService centralGitService;
@Autowired
CentralGitService centralGitService;

Comment on lines +1447 to +1449
GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
localBranches.remove(baseGitMetadata.getRefName());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure null safety for baseGitMetadata

Add null checks for baseGitMetadata and baseGitMetadata.getRefName() to prevent potential NullPointerException.

Comment on lines +693 to +710
ArtifactJsonTransformationDTO jsonTransformationDTO, GitArtifactMetadata baseGitData, GitRefDTO gitRefDTO) {
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType());

String remoteUrl = baseGitData.getRemoteUrl();
String publicKey = baseGitData.getGitAuth().getPublicKey();
String privateKey = baseGitData.getGitAuth().getPrivateKey();

Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(
jsonTransformationDTO.getWorkspaceId(),
jsonTransformationDTO.getBaseArtifactId(),
jsonTransformationDTO.getRepoName());

return fsGitHandler.createAndCheckoutReference(repoSuffix, gitRefDTO);
// TODO: add the checkout to the current branch as well.
return fsGitHandler
.createAndCheckoutReference(repoSuffix, gitRefDTO)
.then(fsGitHandler.pushApplication(
repoSuffix, remoteUrl, publicKey, privateKey, gitRefDTO.getRefName()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null checks for baseGitData and its GitAuth properties

Ensure baseGitData, baseGitData.getGitAuth(), getPublicKey(), and getPrivateKey() are not null to prevent potential NullPointerException.

Copy link

Failed server tests

  • com.appsmith.server.imports.internal.ImportServiceTests#importArtifact_WithoutThemes_LegacyThemesAssigned

@sondermanish sondermanish added the ok-to-test Required label for CI label Jan 22, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)

738-754: Improve error handling in widget serialization.

The error handling for JSON processing could be enhanced with more specific error details.

-    log.error("Error while deserializing widget with file path {}", entry.getKey().getFilePath());
+    log.error("Error while deserializing widget with file path {}. Error: {}", 
+        entry.getKey().getFilePath(), 
+        jsonProcessingException.getMessage());
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1)

Line range hint 3008-3031: Enhance merge status error handling.

The error handling for merge status could be improved with more specific error types.

- if (error instanceof AppsmithException) {
+ if (error instanceof AppsmithException || error instanceof GitAPIException) {
     mergeStatus.setMessage(error.getMessage());
+    mergeStatus.setErrorType(error.getClass().getSimpleName());
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00232d7 and 16381a8.

📒 Files selected for processing (4)
  • app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (25 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (21 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (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-unit-tests / server-unit-tests
  • GitHub Check: server-spotless / spotless-check
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (2)

70-74: LGTM! Imports are well organized.

The imports for Git-related constants are properly organized and follow the standard Java import conventions.


194-196: LGTM! File format version is properly added to metadata.

The file format version is correctly added to the metadata JSON object, which is essential for version compatibility checks.

app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (3)

270-271: LGTM! Improved file path validation.

The file path validation logic is properly enhanced to handle both regular files and empty directories.


297-297: LGTM! Consistent README file handling.

The README file handling is consistent across the codebase using the constant README_FILE_NAME.

Also applies to: 1076-1076


Line range hint 2748-2749: LGTM! Proper branch status handling.

The branch status checks are properly implemented with appropriate error handling.

Also applies to: 2992-2993

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)

2592-2601: LGTM! Well-structured artifact generation method.

The generateArtifactForRefCreation method is well-structured and follows proper permission checks.

Copy link

Failed server tests

  • com.appsmith.server.imports.internal.ImportServiceTests#importArtifact_WithoutThemes_LegacyThemesAssigned

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesITWithCentralService.java (5)

172-175: Follow-up on the TODO.
The comment mentions making the file path relative and exploring the use of the controller layer. Addressing these improvements will help reduce path coupling and can increase the test’s representativeness.

Would you like me to create a follow-up issue or propose a refactor to handle the file path rework?


225-236: Use a more robust waiting strategy for auto-commit.
The manual Thread.sleep(500) approach can introduce flakiness if the system is slow or has intermittent delays. A more resilient solution involves polling a specific reactive endpoint or using built-in Reactor retry/backoff strategies.


457-459: Review local checkout approach before deleting.
The comment wonders if there's reliance on auto-commit for branch switching prior to deletion. Investigate whether forcibly checking out “bar” is the best approach or if a cleanup or forced discard is more appropriate.


540-541: Better clarify merge fail messages.
Test ensures merges fail when local changes exist, but the message might be vague in certain edge cases. Consider clarifying or elaborating the error message for faster root cause analysis.


613-617: Use consistent naming for the auto-commit progress method.
The helper method getAutocommitProgress is descriptive but consider standardizing naming across the codebase (e.g., “getAutoCommitStatus” or “fetchAutoCommitState”).

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16381a8 and 22a669c.

📒 Files selected for processing (1)
  • app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesITWithCentralService.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / client-build / client-build
  • GitHub Check: server-unit-tests / server-unit-tests
  • GitHub Check: server-spotless / spotless-check
🔇 Additional comments (6)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesITWithCentralService.java (6)

60-122: Document and maintain test lifecycle steps in code comments.
The comprehensive docstring meticulously explains each stage of the Git workflow tested. Consider keeping the docstring updated whenever you modify or extend the test flow to ensure new contributors can easily follow the end-to-end sequence.


230-233: Restore the assertion or consider a max retry count for auto-commit.
The test currently comments out the fail("Auto-commit took too long") call, which reduces the test's reliability. If auto-commit stalls, the test won’t fail and might silently pass.


351-354: Confirm that branch protection rules are synchronized.
The restricted branch is assigned, but ensure that any existing local references or ephemeral states reconcile with the server. This is crucial for consistent behavior in team environments.


525-527: Revisit the diff on import side-effects.
A TODO suggests on-page load triggers a diff. Confirm whether these artifact changes are expected and if they're legitimate or can be suppressed for certain test scenarios.


591-610: Check cleanup consistency when disconnecting artifacts.
The test verifies that branches foo, bar, and baz are removed, but check that no leftover references, hooks, or partial states remain.


208-210: Investigate the unexpected extra commit.
A second commit appearing immediately could be caused by an automatic commit hook, a leftover file change, or a mismatch in the test environment.

Use the following script to detect any phantom changes:

## 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 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.Git"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!CAUTION]  
> If you modify the content in this section, you are likely to disrupt
the CI result for your PR.

<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No
@sondermanish sondermanish enabled auto-merge (squash) January 27, 2025 07:16
@sondermanish sondermanish merged commit e89788d into release Jan 27, 2025
43 checks passed
@sondermanish sondermanish deleted the chore/test-it branch January 27, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants