chore: Add code-split for Git integration for packages #39166
Conversation
WalkthroughThis pull request updates several components in the Git integration. In the central Git service, the internal logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant Server as CentralGitServiceCEImpl
participant GitHelper as GitArtifactHelper
participant Logger as ErrorLogger
Server->>GitHelper: Rehydrate artifact from last commit
Server->>GitHelper: validateAndPublishArtifact(artifact, true)
alt Exception occurs
Server->>Logger: Log error details
end
sequenceDiagram
participant TestUtils as GitTestUtilsCE
participant Util as GitArtifactTestUtils
participant Artifact as Artifact
TestUtils->>Util: getArtifactSpecificUtils(artifactType)
Util-->>TestUtils: Return instance (GitApplicationTestUtils)
TestUtils->>Util: createADiff(artifact)
Util-->>TestUtils: Mono<Void> complete signal
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitApplicationTestUtils.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitArtifactTestUtils.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitTestUtils.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ce/GitTestUtilsCE.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (4)
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitArtifactTestUtils.java (1)
6-7: LGTM! Clean interface design.The conversion to a generic interface with a single responsibility is a good design choice, allowing for different implementations per artifact type.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitTestUtils.java (1)
9-12: LGTM! Clean inheritance and DI pattern.Good use of constructor injection and inheritance to extend the CE functionality.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ce/GitTestUtilsCE.java (1)
16-18: Consider handling different artifact types.The method ignores the artifactType parameter and always returns gitApplicationTestUtils. This might need to be updated when supporting different artifact types in the future.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
2058-2058: Improved validation in discardChanges method.The change from
publishArtifacttovalidateAndPublishArtifactadds an additional validation layer before publishing the artifact, which enhances error handling and data integrity.
|
|
||
| Application application = (Application) artifact; | ||
| String pageId = application.getPages().get(0).getId(); | ||
| Plugin plugin = pluginService.findByPackageName("restapi-plugin").block(); |
There was a problem hiding this comment.
Avoid blocking calls in reactive code.
The .block() call breaks the reactive chain and could cause performance issues. Consider using reactive operators instead.
- Plugin plugin = pluginService.findByPackageName("restapi-plugin").block();
+ return pluginService.findByPackageName("restapi-plugin")
+ .flatMap(plugin -> {
+ Datasource datasource = new Datasource();
+ // ... rest of the code
+ return layoutActionService.createSingleAction(action, Boolean.FALSE);
+ })
+ .then();Committable suggestion skipped: line range outside the PR's diff.
| @Autowired | ||
| LayoutActionService layoutActionService; | ||
| @Autowired | ||
| PluginService pluginService; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace field injection with constructor injection.
Field injection is not recommended. Use constructor injection instead for better testability and explicit dependency declaration.
- @Autowired
- LayoutActionService layoutActionService;
- @Autowired
- PluginService pluginService;
+ private final LayoutActionService layoutActionService;
+ private final PluginService pluginService;
+
+ public GitApplicationTestUtils(
+ LayoutActionService layoutActionService,
+ PluginService pluginService) {
+ this.layoutActionService = layoutActionService;
+ this.pluginService = pluginService;
+ }📝 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.
| @Autowired | |
| LayoutActionService layoutActionService; | |
| @Autowired | |
| PluginService pluginService; | |
| private final LayoutActionService layoutActionService; | |
| private final PluginService pluginService; | |
| public GitApplicationTestUtils( | |
| LayoutActionService layoutActionService, | |
| PluginService pluginService) { | |
| this.layoutActionService = layoutActionService; | |
| this.pluginService = pluginService; | |
| } |
…code-split-git-for-packages
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Adding comment to keep the PR active |
Description
EE counterpart: https://github.com/appsmithorg/appsmith-ee/pull/6032
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.ImportExport, @tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13263436476
Commit: b2f06cc
Cypress dashboard.
Tags:
@tag.ImportExport, @tag.GitSpec:
Tue, 11 Feb 2025 13:46:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit