chore: made discard changes async after ref creation#41785
Open
sondermanish wants to merge 1 commit intoreleasefrom
Open
chore: made discard changes async after ref creation#41785sondermanish wants to merge 1 commit intoreleasefrom
sondermanish wants to merge 1 commit intoreleasefrom
Conversation
Contributor
Author
|
/build-deploy-preview skip-tests=true |
Contributor
WalkthroughThis PR introduces an asynchronous event-driven architecture for git discard-changes operations. A new ChangesAsync Discard Event Architecture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/25571612871. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
2342-2355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize nullable
isValidateAndPublishbefore branching.With the new nullable flag,
nullcurrently takes the non-validate path (publishArtifact) atLine 2414, which can silently bypass validation. Default this totrueto preserve legacy behavior.Suggested patch
protected Mono<? extends Artifact> discardChanges( Artifact branchedArtifact, GitType gitType, Boolean isValidateAndPublish, Boolean shouldHydratePackages) { + final boolean validateAndPublish = !Boolean.FALSE.equals(isValidateAndPublish); ArtifactType artifactType = branchedArtifact.getArtifactType(); @@ - if (!TRUE.equals(isValidateAndPublish)) { + if (!validateAndPublish) { return gitArtifactHelper.publishArtifact(artifactFromLastCommit, true); }Also applies to: 2414-2426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java` around lines 2342 - 2355, Normalize the nullable isValidateAndPublish flag to default true before branching: in discardChanges(String branchedArtifactId, ...) (and the other discardChanges overload that takes an Artifact), replace usages of the nullable Boolean with a primitive boolean computed as boolean validateAndPublish = (isValidateAndPublish == null) ? true : isValidateAndPublish; then pass that normalized value (boxed if needed) into the subsequent discardChanges(...) / publishArtifact(...) calls (references: discardChanges(branchedArtifact, gitType, isValidateAndPublish), publishArtifact) so null no longer takes the non-validate path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImpl.java`:
- Around line 31-32: The class GitDiscardChangesAsyncEventManagerImpl is logging
the entire discard event (the 'event' object) which contains PII like
authorName; update the log.info calls that reference 'event' to instead log only
non-PII fields (for example event.getApplicationId(), event.getBranchName(),
event.getCommitId()/change count or a boolean status) and explicitly omit or
mask authorName, and apply the same change to the second log.info in this class
(both occurrences that currently pass the whole event). Use explicit
getters/properties in the log message so only allowed fields are emitted and
avoid serializing the full event object.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImplTest.java`:
- Line 108: The test in GitDiscardChangesAsyncEventManagerImplTest uses a
1-second timeout on the latch (assertThat(latch.await(1,
TimeUnit.SECONDS)).isTrue();) which is flaky under CI; increase the async wait
budget by changing the timeout value to a larger duration (e.g., 5 or 10
seconds) in that assertion so the scheduler-based async path has more time to
complete.
---
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 2342-2355: Normalize the nullable isValidateAndPublish flag to
default true before branching: in discardChanges(String branchedArtifactId, ...)
(and the other discardChanges overload that takes an Artifact), replace usages
of the nullable Boolean with a primitive boolean computed as boolean
validateAndPublish = (isValidateAndPublish == null) ? true :
isValidateAndPublish; then pass that normalized value (boxed if needed) into the
subsequent discardChanges(...) / publishArtifact(...) calls (references:
discardChanges(branchedArtifact, gitType, isValidateAndPublish),
publishArtifact) so null no longer takes the non-validate path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4447ea99-e2f8-41d1-8c31-b1e9e4e2f7ec
📒 Files selected for processing (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/events/GitDiscardChangesEvent.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManager.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImplTest.java
Comment on lines
+31
to
+32
| log.info("published event for git discard changes: {}", event); | ||
| applicationEventPublisher.publishEvent(event); |
Contributor
There was a problem hiding this comment.
Avoid logging full discard event payload (contains user identifiers).
Line 31 and Line 39 log the whole event object, which includes authorName. Prefer structured logs with non-PII fields only.
Suggested patch
- log.info("published event for git discard changes: {}", event);
+ log.info(
+ "Published git discard changes event for artifactId={}, artifactType={}, gitType={}",
+ event.getArtifactId(),
+ event.getArtifactType(),
+ event.getGitType());
@@
- log.info("received event for git discard changes: {}", event);
+ log.info(
+ "Received git discard changes event for artifactId={}, artifactType={}, gitType={}",
+ event.getArtifactId(),
+ event.getArtifactType(),
+ event.getGitType());Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImpl.java`
around lines 31 - 32, The class GitDiscardChangesAsyncEventManagerImpl is
logging the entire discard event (the 'event' object) which contains PII like
authorName; update the log.info calls that reference 'event' to instead log only
non-PII fields (for example event.getApplicationId(), event.getBranchName(),
event.getCommitId()/change count or a boolean status) and explicitly omit or
mask authorName, and apply the same change to the second log.info in this class
(both occurrences that currently pass the whole event). Use explicit
getters/properties in the log message so only allowed fields are emitted and
avoid serializing the full event object.
|
|
||
| manager.discardChangesEventListener(event); | ||
|
|
||
| assertThat(latch.await(1, TimeUnit.SECONDS)).isTrue(); |
Contributor
There was a problem hiding this comment.
Increase async wait budget to reduce test flakes.
Line 108 uses a 1-second latch timeout for a scheduler-based async path; this can intermittently fail under CI load.
Suggested patch
- assertThat(latch.await(1, TimeUnit.SECONDS)).isTrue();
+ assertThat(latch.await(5, TimeUnit.SECONDS)).isTrue();📝 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
| assertThat(latch.await(1, TimeUnit.SECONDS)).isTrue(); | |
| assertThat(latch.await(5, TimeUnit.SECONDS)).isTrue(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/git/central/helpers/GitDiscardChangesAsyncEventManagerImplTest.java`
at line 108, The test in GitDiscardChangesAsyncEventManagerImplTest uses a
1-second timeout on the latch (assertThat(latch.await(1,
TimeUnit.SECONDS)).isTrue();) which is flaky under CI; increase the async wait
budget by changing the timeout value to a larger duration (e.g., 5 or 10
seconds) in that assertion so the scheduler-based async path has more time to
complete.
|
Deploy-Preview-URL: https://ce-41785.dp.appsmith.com |
sondermanish
added
ok-to-test
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.Git"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25646779287
Commit: 5be2cab
Cypress dashboard.
Tags: @tag.Git
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts
List of identified flaky tests.Mon, 11 May 2026 02:37:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests