Skip to content

fix: git pull resulted in a sync commit leading to lost changes#41467

Merged
sondermanish merged 1 commit intoreleasefrom
fix/git-pull
Dec 16, 2025
Merged

fix: git pull resulted in a sync commit leading to lost changes#41467
sondermanish merged 1 commit intoreleasefrom
fix/git-pull

Conversation

@sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Dec 16, 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"

🔍 Cypress test results

Tip

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


Tue, 16 Dec 2025 08:53:56 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes

    • Fixed Git import workflow to correctly apply commits to published artifacts during branched imports, ensuring reliable and consistent behavior.
  • Chores

    • Optimized Git service processing for improved efficiency in artifact handling and commit operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@sondermanish sondermanish requested a review from a team as a code owner December 16, 2025 08:01
@sondermanish sondermanish self-assigned this Dec 16, 2025
@sondermanish sondermanish added the ok-to-test Required label for CI label Dec 16, 2025
@github-actions github-actions bot added the Bug Something isn't working label Dec 16, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

Walkthrough

The Git service import workflow chain is restructured to publish artifacts directly before committing, eliminating an intermediate lookup step. The commit operation now references the published artifact instead of performing an additional fetch.

Changes

Cohort / File(s) Change Summary
Git Artifact Import Chain Refactoring
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
Reordered post-import chain to publish artifact first, then perform commit using published artifact reference. Replaced getArtifactById lookup with direct publishedArtifact usage in commit flow. Commit now uses publishedArtifact ID and type with isFileLock flag set to false. Added TODO comment noting potential async commit capability.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single file change with localized logic restructuring
  • Reordering of operations in a synchronous chain—verify artifact state remains consistent between publish and commit steps
  • Confirm the published artifact reference carries all necessary context for downstream commit operations

Poem

🔄 Artifacts flow in a cleaner line,
Publish first, then commit divine,
No extra lookups in between,
The cleanest Git chain ever seen!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description follows the template structure but lacks critical information including the issue reference and implementation details. Add the issue number/URL that this PR fixes, provide a clear description of the problem and solution, and include relevant context about the git pull changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: preventing data loss from sync commits during git pull operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/git-pull

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sondermanish
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20260747558.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 41467.
recreate: .

@github-actions
Copy link

Deploy-Preview-URL: https://ce-41467.dp.appsmith.com

@github-actions
Copy link

Failed server tests

  • com.appsmith.server.helpers.UserUtilsTest#makeInstanceAdministrator_WhenUserAlreadyAdmin_MaintainsPermissionsSuccessfully

@sondermanish sondermanish enabled auto-merge (squash) December 16, 2025 09:44
@sondermanish sondermanish merged commit ff36efa into release Dec 16, 2025
55 of 57 checks passed
@sondermanish sondermanish deleted the fix/git-pull branch December 16, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working ok-to-test Required label for CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants