fix(git): strip only leading origin prefix#41793
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughConsolidates origin/ prefix stripping into a package-private static stripOriginPrefix() helper and replaces inline replaceFirst(...) calls in both CentralGitServiceCEImpl and CommonGitServiceCEImpl; adds unit tests validating the helper. ChangesGit Reference Normalization Consolidation
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
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/common/CommonGitServiceCEImpl.java (1)
2913-2916:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse leading-prefix strip here to avoid branch-name corruption
This path still strips all
origin/occurrences viareplace, soorigin/feature/origin/mainbecomesfeature/main. It should only remove a leading prefix.Suggested fix
- String branchName = gitBranchDTO.getBranchName().replace("origin/", ""); + String branchName = stripOriginPrefix(gitBranchDTO.getBranchName());🤖 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/common/CommonGitServiceCEImpl.java` around lines 2913 - 2916, The code in CommonGitServiceCEImpl uses gitBranchDTO.getBranchName().replace("origin/", "") which removes every occurrence of "origin/" instead of only the leading prefix; change this to strip only a leading "origin/" (e.g., use replaceFirst("^origin/", "") or substring after checking startsWith, or StringUtils.removeStart) when computing branchName so inputs like "origin/feature/origin/main" become "feature/origin/main" and not "feature/main".
🤖 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.
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java`:
- Around line 2913-2916: The code in CommonGitServiceCEImpl uses
gitBranchDTO.getBranchName().replace("origin/", "") which removes every
occurrence of "origin/" instead of only the leading prefix; change this to strip
only a leading "origin/" (e.g., use replaceFirst("^origin/", "") or substring
after checking startsWith, or StringUtils.removeStart) when computing branchName
so inputs like "origin/feature/origin/main" become "feature/origin/main" and not
"feature/main".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed73c785-49d8-4cbe-bc85-38ac7ec62fed
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/git/central/CentralGitServiceCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/git/common/CommonGitServiceCEImplTest.java
|
Follow-up pushed: Addressed the CodeRabbit finding by changing the remaining branch-listing call site in Validation: git diff --check |
Description
Tip
TL;DR — Completes the
origin/prefix-normalization follow-up from #41758 so branch names that containorigin/after the prefix are not corrupted.Related to #41758.
#41758 introduced
stripOriginPrefix(...)inCentralGitServiceCEImplbecausereplaceFirst(ORIGIN, "")can removeorigin/from the middle of a ref name. Its follow-up notes called out the remaining Central/Common service sites for a separate sweep.This PR applies the same anchored-prefix behavior to the remaining
CentralGitServiceCEImplandCommonGitServiceCEImplpaths that were still usingreplaceFirst(ORIGIN, "")orreplace(ORIGIN, ""). For example:origin/feature/origin/mainstill normalizes tofeature/origin/mainfeature/origin/mainstaysfeature/origin/mainIt also adds focused unit coverage for the shared helper behavior in both service packages.
Automation
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Tests
git diff --checkmvn -pl appsmith-server -Dtest=CentralGitServiceCEImplTest,CommonGitServiceCEImplTest -DskipITs testbecause this environment does not havemvninstalled.Summary by CodeRabbit
Refactor
Tests