chore: Switching to ref types and names for git#38433
Conversation
WalkthroughThis comprehensive pull request introduces a significant refactoring of the Appsmith codebase, focusing on transitioning from a branch-based reference model to a more generalized reference type system. The changes span multiple modules and classes, primarily replacing Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Domain
Client->>Controller: Request with refType/refName
Controller->>Service: Pass refType and refName
Service->>Repository: Query with refType and refName
Repository->>Domain: Fetch domain object
Domain-->>Repository: Return object
Repository-->>Service: Return result
Service-->>Controller: Process result
Controller-->>Client: Respond with data
Suggested labels
Suggested reviewersPoem
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
Failed server tests
|
Failed server tests
|
| .findByBaseIdBranchNameAndApplicationMode(application.getId(), defaultBranchName, mode) | ||
| .zipWith(newPageService.findByRefTypeAndRefNameAndBasePageIdAndApplicationMode( | ||
| RefType.BRANCH, defaultBranchName, basePageId, mode)); | ||
| RefType.branch, defaultBranchName, basePageId, mode)); |
There was a problem hiding this comment.
Constant assuming we'll never need to run consolidated for tags
There was a problem hiding this comment.
why is that the case? what about tagged artifacts?
There was a problem hiding this comment.
Default branch would still always be branch no?
| return newPageService | ||
| .findByRefTypeAndRefNameAndBasePageId( | ||
| RefType.BRANCH, refName, defaultPageId, pagePermission.getReadPermission(), null) | ||
| RefType.branch, refName, defaultPageId, pagePermission.getReadPermission(), null) |
There was a problem hiding this comment.
Constant assuming we'll never need to migrate DSL for tags. Gotta figure out how to manage version mismatch in this case though.
There was a problem hiding this comment.
This one's still under discussion, TBD
Failed server tests
|
| } | ||
|
|
||
| @Override | ||
| public Mono<ActionCollection> findByBaseIdAndBranchName(String id, String branchName) { |
There was a problem hiding this comment.
Where are you putting all these methods?
There was a problem hiding this comment.
They're not used, deleting what need not be modified
Failed server tests
|
...r/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
nidhi-nair
requested review from
a team,
AnaghHegde,
abhvsn and
sharat87
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (59)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration057MoveDRToBaseIds.java (2)
40-41: Consider referencing constants instead of string literals.
Using"branchName"directly can be brittle. Introducing a static constant (e.g.,RefAwareDomain.Fields.branchName) or referencing an existing field helps avoid typos and simplifies future maintenance.
73-75: Ensure consistent referencing for future maintainability.
You're assigningRefAwareDomain.Fields.baseIdwhile still using the string literal"branchName". If the domain evolves, referencing a dedicated field constant (e.g.,RefAwareDomain.Fields.branchName) could prevent mistakes and further unify the code.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (2)
139-139: Consider providing a default fallback if both values are empty
IfgitArtifactMetadata.getDefaultBranchName()is empty or null, the code falls back togitArtifactMetadata.getRefName(). However, ifrefNameis also empty or null, the method will return a blank string. You may wish to provide a sensible default or handle this scenario explicitly.
152-153: ReusegetDefaultBranchNamefor consistency
Here, the logic checksmetadata.getRefName()and compares it withmetadata.getDefaultBranchName(). For better consistency, consider reusing thegetDefaultBranchName(...)method to hide the fallback details and then compare.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java (1)
105-106: Consider adding validation for ref fieldsThe implementation looks correct, but consider adding validation for refType and refName values before setting them.
+ if (StringUtils.isEmpty(importingMetaDTO.getRefType()) || StringUtils.isEmpty(importingMetaDTO.getRefName())) { + throw new AppsmithException(AppsmithError.INVALID_PARAMETER, "Reference type and name cannot be empty"); + } actionCollection.setRefType(importingMetaDTO.getRefType()); actionCollection.setRefName(importingMetaDTO.getRefName());app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.java (1)
138-138: Consider referencing the providedbranchNameparameter.Currently, the code passes
nullexplicitly tofindExistingArtifactByIdAndBranchNameand setsrefNametonull, making thebranchNameparameter unused. If your intent is to dropbranchNamecompletely in favor ofrefName, consider updating the method signature and removing or repurposing thebranchNameparameter for clarity.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (1)
73-78: Align documentation with new parametersThe ref-based parameters (
refType&refName) replace the oldbranchNameusage, which successfully decouples the service from branch-based assumptions. However, consider adding or updating Javadoc comments to clarify under what circumstances eachRefTypeis used (e.g., tag, branch, etc.) to help maintainers understand the new paradigm.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1)
447-447: Consider providing a named parameter or comment instead ofnullfor clarity.
Using a named constant or a short comment can help future maintainers quickly understand whynullis passed here.app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java (2)
199-199: Method naming mismatch.The method name still references
branchNameinexportByArtifactIdAndBranchName, but the code now usesrefName. Consider renaming for clarity.- .exportByArtifactIdAndBranchName( + .exportByArtifactIdAndRefName(
211-211: Rename local variable for clarity.
branchNameis now storingrefName. Rename the variable accordingly.- String branchName; + String refName;app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java (1)
151-152: Consider making these variables immutable.
SincerefNameandrefTypeare assigned once and never reassigned, you could declare them asfinalfor clarity and to emphasize that they do not change.- String refName = null; - RefType refType = null; + final String refName; + final RefType refType;app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)
651-657: Consider handling the case whenrefNameis null.
While this approach avoids aNullPointerExceptionby checking ifapplication.getGitArtifactMetadata()is null,getRefName()itself may still return null. IfrefNameremains null, consider adding a fallback or debug logging for better traceability.app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java (1)
68-68: Nice consistency with the refName change!
This update aligns with the broader transition to using generic "ref" terminology. As a minor detail, consider renaming the constantBRANCH_NAMEtoREF_NAMEfor an even cleaner and self-explanatory codebase.You could apply a quick rename as follows:
- private static final String BRANCH_NAME = "develop"; + private static final String REF_NAME = "develop";app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (1)
185-192: Good use of logical OR and AND queries for distinct references.
By checking both the oldbranchNamefield and the new pair ofrefName&refType, you're ensuring backward compatibility. Keep in mind that eventually you may remove thebranchNamecheck if it’s deprecated.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCE.java (1)
11-11: Ensure parameters are well-documented.
As this method signature has multiple parameters, consider adding or enhancing Javadoc to improve code readability and clarify the purpose of each argument, especiallyrefTypeandrefName.app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java (2)
470-470: Consider removing duplicated references to the same Mono
Using the samebranchedResultMonofor both T1 and T3 in theMono.zip()tuple can be confusing. If the intention is to verify two different states or transformations, clarifying them in separate Monos may improve readability.
473-473: Naming clarity
RenamingactionDTOor the variable assigned toT3might help indicate what’s being tested, especially since it refers to the same Mono as T1.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1)
123-123: Consider renaming the parameter to maintain consistency.The parameter is still named
branchNamebut is being stored asrefName. For clarity, rename the parameter torefName. This ensures consistency and avoids confusion between old and new naming conventions.-public Mono<Application> createNewArtifactForCheckout(Artifact sourceArtifact, String branchName) { - sourceBranchGitData.setRefName(branchName); +public Mono<Application> createNewArtifactForCheckout(Artifact sourceArtifact, String refName) { + sourceBranchGitData.setRefName(refName);app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
710-710: Clarify error message to match the updated naming.Here, the code checks
refNamebut the error message still referencesFieldName.BRANCH_NAME. Consider updating for consistency, as shown below:-if (!hasText(refName)) { - return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); +if (!hasText(refName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "refName")); }
757-757: Mention refName consistently in error messages.As with the earlier validation block, consider updating the parameter name in the error string:
-if (!hasText(refName)) { - return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); +if (!hasText(refName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "refName")); }app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (3)
456-457: Duplicate assignment ofrefTypeandrefName.
These lines mirror the logic in lines 129–130 and elsewhere (e.g., lines 484–485). Centralizing this repetitive code in a small helper method would adhere to DRY principles.
484-485: Further duplicate assignment ofrefTypeandrefName.
Consider refactoring to maintain consistent assignment across newly created or updated pages.
507-508: Repetitive reference assignment.
This is the third occurrence of settingrefTypeandrefName. A small utility method here may simplify maintenance.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
87-87: Consider adding Javadoc for new parametersThe additional parameters
refTypeandrefNamesignificantly change how unpublished actions are fetched. Documenting their use would help maintain clarity.app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java (2)
87-87: Consider updating the error message to reflectrefName.Currently, the error references
FieldName.BRANCH_NAME, which may be inconsistent with the new naming:- return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.REF_NAME));
102-102: Review log statements to ensure naming consistency.You currently log: “branch name: {}, …” while referencing
refName. Consider unifying this terminology to avoid confusion.Also applies to: 115-115, 124-124, 132-132, 140-140
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java (1)
217-217: Consider renamingexportByArtifactIdAndBranchNamefor clarity.
Since you're passinggitData.getRefName(), a method name that still references "branch name" can be confusing.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (1)
1285-1285: Consider renaming the parameter torefName.
The analytics event still referencesBRANCH_NAME, yet the method now passesbranchedPage.getRefName(). This might be confusing to future maintainers.-private Mono<Application> sendPageOrderAnalyticsEvent( - Application application, String pageId, int order, String branchName) { +private Mono<Application> sendPageOrderAnalyticsEvent( + Application application, String pageId, int order, String refName) { ... - FieldName.BRANCH_NAME, defaultIfNull(branchName, "")); + FieldName.BRANCH_NAME, defaultIfNull(refName, "")); }app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (2)
547-552: Check if projected fields suffice for future expansions.
Currently, you only retrieveidandapplicationId. If you need further fields later, consider updating the projection list.
567-569: Potentially refine the error message.
Revealing the concatenated basePageId and refName might leak some internal data; consider a generic approach if privacy is a concern.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ImportingMetaDTO.java (1)
26-28: Well-structured reference fields.
The addition ofrefTypeandrefNameeffectively replaces the older branch-based design. Consider adding minimal Javadoc to clarify possibleRefTypevalues and how they differ from each other if you find it beneficial.app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (1)
234-234: Check for consistent logging
The logging message for auto-commit is succinct. Consider adding an explicit mention of the branch name for clarity in debugging.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (4)
490-490: Validate therefNamestring for better resilienceHere, the logic checks that
refNameis not empty. Also consider verifying thatrefNamedoesn't contain invalid characters or whitespace to avoid possible errors later on.
512-512: Promote self-documenting variable naming
gitData.getRefName()is clear, but be mindful that multiple references exist. A more explicit naming (liketargetRefName) may enhance readability throughout.
592-592: Warn users on resets with a relevant messageYou call
resetHardhere. Consider logging or throwing a specialized warning to ensure users understand that local changes may be discarded.
597-597: Clarify error message for better debuggingAppending
gitMetadata.getRefName()is good. Also consider including the actual reason for the rejected push, so users can quickly diagnose issues.app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (1)
509-509: Update comment to reflect method usageYou've replaced
getBranchName()withgetRefName(). Ensure inline comments or references in docstrings are similarly updated to avoid confusion.app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java (2)
237-237: Simplify local references if unused
branchNameis set here usinggetRefName(). If you don’t reuse it outside the test, consider localizing to the single scope.
1118-1119: Avoid ref duplication in code pathsSimilar to the prior block, ensure a single source of truth for Git references. If these lines are repeated often, consider a small helper method.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
Line range hint
1229-1229: Consider param usage over hardcoded"testBranch"
You might fetch this from a test environment variable or pass it in, for flexibility.app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (3)
796-796: Use caution with hard-coded string
This is presumably a clear test scenario, but consider a constants file.
1191-1191: Reinforce usage of a single source
Same logic: if"defaultBranch"is repeated often, a central constant might be simpler.
1229-1229: Hard-coded"testBranch"
Similar to above: consider a test utility constant so it’s unified.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (7)
1586-1586: Referring to"master"
Consider whether your project defaults to a different branch name (e.g.,"main") if that is the new standard in your repository.
1660-1660: Referring to"master"
Similar consideration applies here if the default branch is no longer"master".
1758-1758: Referring to"master"
As with earlier lines, you may want to confirm that this branch name matches your current repo settings.
3846-3846: Using"master"in Child Branch Tests
Double-check branch naming conventions if your repo default is not"master".
3937-3937: Setting"master"for Another Micro-test
The usage remains valid, but reevaluate naming consistency if needed.
4047-4047: Assigning"master"togitData.setRefName
Similar note regarding default branch naming applies.
4158-4158: Continuing"master"Usage
As a final note, verify that"master"is intended across your codebase.app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (8)
232-232: Use a single string check method.
Minor note: the code usesStringUtils.isEmptyOrNull()in some places andhasText()in others. Consider standardizing for consistency.
519-519: Metadata naming mismatch.
UsingBRANCH_NAMEas the field key but populatingrefNamemay cause confusion. Consider updating the key to match the refName naming.
929-929: Consider renaming local variable.
The local variable is namedbranchNamebut retrievesgetRefName(). For clarity, rename it torefName.
1195-1195: Mixed string validation methods.
Some lines usehasText(), others useisEmptyOrNull(). Consider a single approach for uniformity.
1419-1419: baseBranchName variable name.
To avoid confusion, consider renaming it tobaseRefNameto align with the new naming convention.
1804-1804: Variable naming consistency.
Currently namedbranchNamebut referencesgetRefName(). Consider renaming torefNamefor clarity.
2267-2267: Local variable naming mismatch.
To align with the new naming scheme, consider naming this local variablerefName.
2781-2781: Naming consistency recommended.
The variable iscurrentBranchbut referencesgetRefName(). ConsidercurrentRefNamefor clarity.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
554-556: Improve log message wording
Rewrite the log statement for clarity, for example:
"Application or page for basePageId {} and refName {} was not found."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/RefType.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/Datasource.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/RefAwareDomain.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ce/ActionCE_DTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Context.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ActionCollectionCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/CustomJSLibCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/GitArtifactMetadataCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/NewActionCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ClonePageMetaDTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ExportingMetaDTO.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ImportingMetaDTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageDTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/IntegrationCE_DTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/partial/PartialExportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java(4 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/common/CommonGitServiceCEImpl.java(25 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration057MoveDRToBaseIds.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(12 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java(3 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java(11 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/exports/internal/ExportServiceTests.java(8 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java(27 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitConnectTests.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java(19 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/searchentities/SearchEntitySolutionCETest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java(5 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java(4 hunks)
⛔ Files not processed due to max files limit (9)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialExportServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java
💤 Files with no reviewable changes (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
✅ Files skipped from review due to trivial changes (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/RefType.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java
🔇 Additional comments (258)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java (2)
3-3: The new import shows a clear intent to rely on ref types rather than branch-specific logic.
That's a good approach, providing greater flexibility for future expansions beyond branches.
193-193: Double-check null handling forbranchName.
Since you're now passingRefType.branchexplicitly, ensure that upstream or downstream functions handle anullor emptybranchNamegracefully, especially in the repository or service layer.app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/partial/PartialExportServiceCEImpl.java (1)
75-75: Consider verifying the intentionality of settingrefNametonull.
If your goal is to retain Git reference information, you may want to supply the actual reference name rather thannull. Confirm that this won't hinder other parts of the code dependent onrefName.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration057MoveDRToBaseIds.java (1)
4-4: Good import change to use RefAwareDomain.
This aligns the migration context with the new reference-based refactoring consistently.app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java (4)
39-39: Variable naming looks consistent.
Good job updating the variable name torefNamefor improved clarity and consistency with ref types.
Line range hint
47-58: Check for potential NPE or empty reference name.
SincerefNameis used in a logging statement, consider validating it to avoidnullor empty string scenarios.
96-96: Naming convention aligns with git ref usage.
Good approach migrating frombranchNametorefName, ensuring consistent naming for reference types.
114-117: Logging includesrefNameconsistently.
Well done usingrefNamein the logs. This helps troubleshooting by providing clarity on which ref is impacted.app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitConnectTests.java (2)
335-335: Everything looks good here.
Your assertion for therefNamealigns with the refactoring changes.
364-364: Good alignment with new naming convention.
This line correctly setsrefName, ensuring it's consistent with the rest of the ref-based approach.app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (4)
3-3: Excellent use of the new reference type import.
TheRefTypeimport is appropriately introduced to support the changes toupdateDependencyMap.
430-430: Validate the choice ofRefType.branch.
Confirm that each test scenario requiring the branch reference type is correctly handled. Also verify that other reference types, if needed, are covered in separate tests.
466-466: Ensure consistency across publish workflow.
PassingRefType.branchmaintains consistency with how the dependency map is updated before publishing. Check if additional reference types need coverage or if edge cases may arise whenRefType.branchis mismatched.
503-503: Confirm null dependency handling logic.
Passing a nulldependencyMapwithRefType.branchappears consistent. Verify that the null case is properly handled downstream inupdateDependencyMap.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java (1)
105-106: Verify consistent implementation across importable servicesLet's verify that this change pattern is consistently implemented across other importable services.
✅ Verification successful
Let me search for similar patterns in the codebase to verify the consistency of the implementation.
Let me check if the same pattern is implemented in the other application importable service.
Implementation is consistent across importable services
The pattern of setting
refTypeandrefNamefromimportingMetaDTOis consistently implemented in bothActionCollectionApplicationImportableServiceCEImplandNewActionApplicationImportableServiceCEImpl, maintaining uniform behavior across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar service implementations to ensure consistent refactoring ast-grep --pattern 'class $_ApplicationImportableServiceCEImpl implements ArtifactBasedImportableServiceCE { $$$ createNewResource($_, $_, $_) { $$$ } }'Length of output: 165
Script:
#!/bin/bash # Search for files containing ApplicationImportableServiceCEImpl in their names fd "ApplicationImportableServiceCEImpl.java" # Search for implementations of createNewResource method with refType and refName rg -A 5 "createNewResource.*\(" --type javaLength of output: 6385
Script:
#!/bin/bash # Let's check the implementation in NewActionApplicationImportableServiceCEImpl.java to verify if it sets refType and refName rg -B 5 -A 10 "setRefType|setRefName" app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.javaLength of output: 778
app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (3)
80-81: Ensure consistent usage throughout codebaseRenaming the method to incorporate
refTypeandrefNameis consistent with the new ref-based approach. Verify all references and transitive calls using this method (e.g., tests, service, or downstream code) have been updated accordingly to avoid mismatched method signatures.
83-83: Method name clarity
findRefPageIdstrongly reflects the updated ref-based strategy, providing improved clarity. This method name aligns well with the domain changes.
94-95: Validate dependency mapping logicSwitching to
RefType refType, String refNameis a logical step to unify dependency updates under the newly introduced reference model. Ensure that all code paths whereupdateDependencyMapis called correctly handle these parameters, especially for cases where the user might switch between different refs or revert from a tag-ref to a branch-ref.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (2)
12-12: New import looks correct.
This import is necessary for accessing the new GitArtifactMetadata functionality.
498-504: RefType and refName metadata assignment
This addition ensures that any imported artifact now properly retains and conveys its Git reference data. Make sure consuming methods handle scenarios where refType or refName might not be present.app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java (2)
190-190: Good consistency in setting refName.Setting
refNameto"actionServiceTest"aligns well with the updated naming convention.
203-203: Confirm consistent ref usage.Ensure this import method treats
refNameconsistently across the codebase.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java (6)
7-7: No issues with the new import.
This import is consistent with the usage ofRefTypein the updated logic.
154-156: Logic for setting reference fields looks good.
It's straightforward to check ifbranchedPage.getRefName()is present before settingrefNameandrefType. No concerns here.
163-172: Ensure consistent handling of optional ref fields in state.
WhenrefNameis not present, the state omitsrefType. Double-check that any downstream parsing of the state gracefully handles missing fields.
361-361: Double-check splitState array length safety.
Here, the logic assumessplitStatehas at least 7 parts forrefName. Confirm that upstream code always constructs this string with the correct length and ordering.
556-558: Passing ref details from branched context is appropriate.
Reusing therefNameandrefTypefrom the branched page ensures correct redirection. Implementation appears correct.
570-572: Consistent integration with ref details.
StoringrefNameandrefTypeinIntegrationDTOextends the same logic used in other parts of the code. No issues found.app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (2)
144-144: Well-aligned switch to a more flexible reference naming.
This change fromsetBranchNametosetRefNameis consistent with the new ref-based approach, ensuring the code supports various Git reference types.
650-650: Uniform application ofrefNameusage.
Thanks for maintaining consistency by also changingtestApplication.getGitApplicationMetadata().setBranchName("another-branch-name")tosetRefName. Everything looks good here.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (3)
134-135: Nice integration ofrefTypeandrefNameattributes here!
These fields expand the code's querying capabilities, making it more generic and agnostic of specific Git branch usage.
174-179: Method signature rename expands flexibility for referencing pages.
RenamingfindPageByBranchNameAndBasePageId(...)tofindPageByRefTypeAndRefNameAndBasePageId(...)is a proper approach toward a more generalized reference system.
194-194: Ensuring null checks for bothbranchNameandrefNameis prudent.
This guard condition helps differentiate between "no reference provided" and an explicitly set reference.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (1)
36-41: Method signature update aligns with the new reference model.
ReplacingbranchNamewith(RefType refType, String refName)is consistent and maintains clarity in the codebase.app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryTest.java (1)
103-104: Test method now covers nullrefTypeandrefName.
Switching fromfindPageByBranchNameAndBasePageIdtofindPageByRefTypeAndRefNameAndBasePageId(null, null, ...)ensures we test scenarios with no specified reference type or name, validating the new approach.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCE.java (1)
3-3: Confirm consistent usage of newRefType.
UsingRefTypefor versioning references is a strong architectural move, but ensure that all invocations and references in downstream services or controller layers have been updated accordingly.✅ Verification successful
RefTypeusage is consistent across the codebaseThe search results confirm that all invocations of
getConsolidatedInfoForPageLoadhave been properly updated to use theRefTypeparameter:
- The service interface and implementation (
ConsolidatedAPIServiceCE.javaandConsolidatedAPIServiceCEImpl.java) correctly define the method withRefType- The controller layer (
ConsolidatedAPIController.java) properly passes theRefTypeparameter- Test cases in
ConsolidatedAPIServiceImplTest.javademonstrate correct usage withRefType.branch🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for all references to getConsolidatedInfoForPageLoad to ensure they're updated to use the RefType variant rg -A 3 'getConsolidatedInfoForPageLoad'Length of output: 9895
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/NewActionCE.java (3)
8-8: Adopting the new reference-aware domain approach
ImportingRefAwareDomainaligns well with the refactor objective to handle git references more flexibly. This change keeps the code consistent with the new reference model introduced across the codebase.
23-23: Switching the superclass toRefAwareDomain
ExtendingRefAwareDomaininstead ofBranchAwareDomainis a clear and appropriate refactor toward the new architecture. This enhances flexibility for future git reference enhancements.
64-64: AdoptingRefAwareDomain.Fieldsin nested class
HavingFieldsextendRefAwareDomain.Fieldsmaintains consistency with the top-level class and ensures that the primary attributes for handling git references are accessible throughout the domain structure.app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java (2)
490-490: No issues found
Ensuring the branched Action’s baseId equals its own id here appears correct.
492-492: Validation of refName
Checking thatactionDTO.getRefName()equals"testBranch"correctly verifies the updated reference name in branched actions.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (3)
263-264: Confirm null assignments or consider valid fallback.The code explicitly sets both
refTypeandrefNametonull. Ensure that subsequent logic can handlenullreferences correctly. Otherwise, consider providing a valid fallback.
276-277: Maintain consistency with clearing references.As with pages, you’re clearing out
refTypeandrefNamein actions, so be mindful of any code that might still rely on their presence. Confirm that the calling methods are properly handlingnullvalues.
[approve]
340-340: Correct approach to compare enumeration constants.Using
.equals()onRefType.tagis valid, assuming thetagconstant was intentionally converted to lowercase in the enum. Confirm that other references follow the same naming convention to avoid misalignment.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (4)
699-699: Nice consistency with the new refName approach.Renaming
branchNametorefNamealigns well with the shift toward more generic Git references.
719-719: RefName usage is consistent.Switching from
branchNametorefNamehere is appropriate and aligns with the new Git reference model.
746-746: Good consistency in getPageDslVersionNumber as well.This usage of
refNameis consistent with the overall refactoring.
773-773: No further issues here.This call now consistently uses
refName, matching the broader ref-based logic.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (4)
129-130: SettingrefTypeandrefNamefor new pages.
No issues found; this change aligns with the new reference-based model.
479-479: CallingsaveNewPageAndUpdateBaseId.
This approach cleanly delegates page creation logic. Looks good.
495-495: ReturningsaveNewPageAndUpdateBaseId.
Straightforward approach for handling new page creation. No issues.
505-505: Private method signature updated.
Switching from aString branchNametoImportingMetaDTOis suitably flexible. No concerns.app/server/appsmith-server/src/test/java/com/appsmith/server/searchentities/SearchEntitySolutionCETest.java (1)
251-251: Switch to the new Git reference naming convention
ChangingsetBranchNametosetRefNamealigns with the broader ref-based approach for Git references, streamlining maintenance and consistency.Use this script to confirm that there are no remaining references to
setBranchName:app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (2)
3-3: Import looks goodThis import is aligned with the new ref-based approach. Ensure that other components referencing
RefTypeare updated consistently.
93-93: Ensure consistent usage across overloaded methodsBoth overloads of
getUnpublishedActionsnow acceptrefTypeandrefName. Verify that upstream callers use the correct method to avoid confusion.app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java (2)
85-85: Renaming variable torefNameis consistent with the new approach.Looks good.
95-95: Use ofrefNameingetAutoCommitProgressis correct.No issues spotted here.
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java (2)
210-210: Consistent rename tosetRefName.
This change aligns with the move from a branch-centric approach to a more general ref-based approach.
222-222: Double-check method arguments.
importArtifactInWorkspaceFromGitlikely expects a branch name, but we’re providing a ref name. Verify consistency across code references.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (3)
180-180: Check for potential NullPointerException when passing a null branch argument
Please ensure the rest of the import logic safely handlesbranchNameasnullwithout causing unintended side effects in the import process.
204-204: Ref type assignment looks correct
The logic to setrefTypefrom the page appears consistent with the new reference-based model.
205-205: Ref name assignment looks correct
Similarly, settingrefNamefrom the page aligns well with the updated import flow.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (5)
4-4: Looks Good: ImportingRefTypeis appropriate.
No concerns; the import reflects the new reference-based approach.
170-171: Nice addition for setting page references.
This correctly sets the page’srefTypeandrefNameusing the application’s Git metadata, avoiding null-pointer errors by first checkinggetGitArtifactMetadata().
314-319: Potential mismatch betweenrefTypeusage and method parameter.
The methodgetPageAndMigrateDslByBranchAndBasePageIdalways usesRefType.branch, which may be inconsistent if a tag reference is passed. Confirm whetherrefNamecan represent a tag or other ref types and, if so, consider makingrefTypedynamic.
582-583: Cloning page reference fields looks correct.
Copying therefTypeandrefNamefrom the source page ensures that the cloned page’s metadata remains consistent.
745-745: Verify handling of non-default branches or tags.
WhengetRefName()differs fromgetDefaultBranchName(), you look up the default artifact. Double-check logic for a scenario where a tag is in use or if the branch name doesn’t match the default.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (6)
4-4: Import for RefType looks good.
It aligns with the new reference mechanism introduced in this PR.
167-168: Verify null or default usage for refType and refName.
Make sure that both values are handled correctly when the client does not supply a valid reference.
511-516: Method gracefully handles empty basePageId/refName.
Good usage of conditional checks and fallback to fetch the page by ID; ensures backward compatibility with existing logic.Also applies to: 520-520, 529-533
537-538: Ref-type parameters in method signature align with new approach.
Clear and consistent usage of the parameters relevant to the new Git reference workflow.
558-559: Double-check fallback behavior for refName and basePageId.
If both are empty, consider clarifying the error message or adding explicit validation.
629-633: Ensure null-safety for the combined logic referencing refName.
WhenrefNameis null, the method falls back to the default path. Confirm no edge cases are missed (e.g., empty string).app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/RefAwareDomain.java (7)
3-3: Imports look good.
No issues observed with these additional imports.
13-13: Renaming toRefAwareDomain
This aligns well with handling references beyond just branches.
21-22:refTypeintroduced
This field neatly captures the reference type. Fallback logic ingetRefType()ensures safe defaults.
24-25:refNamefield
Allows flexible naming for references, decoupling from branch-only handling.
27-36: Check null return ingetRefType()
Returningnullcould be risky if callers assume a non-null. Consider verifying usage or using a dedicated fallback.
38-40: Fallback logic ingetRefName()
Good approach for backward compatibility by returningbranchNameifrefNameisnull.
42-47: Conditional branchName update insetRefName()
This depends onrefTypebeing set beforehand. Consider verifying consistency ifrefTypechanges downstream.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ActionCollectionCE.java (3)
4-4: Switch toRefAwareDomain
Import and usage are consistent with the new reference model.
24-25: ExtendingRefAwareDomain
A suitable replacement forBranchAwareDomain, aligning with universal references.
55-55:FieldsextendsRefAwareDomain.Fields
Ensures that field constants remain aligned with the new reference-aware structure.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java (3)
3-3: Added import forRefAwareDomain
Moving from branch-focused to reference-aware is consistent.
21-21:NewPageextendsRefAwareDomain
This provides a unified approach for reference handling across pages.
69-69:FieldsextendsRefAwareDomain.Fields
Correct approach for referencing the extended field names in the new domain.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/GitArtifactMetadataCE.java (5)
30-30: RefType field
UsingRefTypefor internal reference representation is consistent with the rest of the refactor.
33-33: StringrefName
The newrefNamefield aligns with the domain model, accommodating non-branch references.
130-132:getRefType()fallback
ReturningRefType.branchby default is a safe approach to avoid null references.
139-139:getRefName()
Gracefully falls back tobranchName, ensuring backward compatibility.
144-144: SynchronizingbranchNameandrefName
Good choice for consistency, but ensure future changes inbranchNamewon't conflict withrefName.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (3)
160-161: Transferring page ref data
SettingrefTypeandrefNamefrom the page domain is consistent with the ref-based approach.
346-347: New actions also adoptrefTypeandrefName
Ensures uniform reference usage across action collections.
385-388: Enhanced debug logs
IncludingrefTypeandrefNamein the log helps identify reference-related issues quickly. Confirm no sensitive data is leaked.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ExportingMetaDTO.java (1)
18-18: Good introduction of therefNamefield.
This clearly aligns with the shift from branch-based to reference-based naming.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ImportingMetaDTO.java (1)
3-3: Nice addition ofRefTypeimport.
This import is necessary for the newly introduced ref-based approach.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/IntegrationCE_DTO.java (2)
3-3: Import ofRefTypeis consistent with the new ref-based model.
Ensures a more flexible handling of Git references.
42-44: Refactoring to useRefTypeandrefNameis a solid choice.
These fields provide better clarity and extendibility than a single branch field.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java (5)
5-5: ReplacingBranchAwareDomainwithRefAwareDomain.
This import aligns with the shift to a more generalized reference model across the application.
30-31: Casting resources toRefAwareDomain.
Ensures that subsequent operations onrefTypeandrefNameare valid. Good approach.
33-34: AssigningrefTypeandrefNamefromimportingMetaDTO.
Accurately populates the domain objects with the correct Git reference data. Looks good.
36-36: UsingrefAwareRefResource.getBaseId().
This ensures consistency in referencing the base entity when the artifact is Git-synced.
39-39: Falling back to the resource’s own base ID.
Good fallback logic to avoid potential null references or incomplete metadata.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageDTO.java (2)
82-83: Good use of typed references!
DefiningrefTypeas aRefTypeenum is a clear and expressive way to handle multiple reference types.
84-86: Assess necessity of transient ref fields
Using@Transientwith@JsonView({Views.Internal.class})could imply these fields won't persist to the database but will serialize to internal responses. If this is the desired behavior, everything looks consistent.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java (1)
35-36: Consistent data model usage
ReplacingbranchNamewithrefTypeandrefNamealigns this service with the newly adopted reference model. This should ensure easier maintenance and integration across the codebase.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/CustomJSLibCE.java (1)
127-127: Transition to RefAwareDomain
Switching fromBranchAwareDomain.FieldstoRefAwareDomain.Fieldsis a clean change that unifies the approach for handling references. This is consistent with the refactoring across the codebase.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (1)
105-106: Ref-based naming consistency
UsingsetRefTypeandsetRefNameinstead ofsetBranchNameis straightforward and consistent with the new reference model.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/Datasource.java (1)
155-155: Update to RefAwareDomain.FieldsSwitching from
BranchAwareDomain.FieldstoRefAwareDomain.Fieldsis consistent with the new reference model changes in this PR. No issues detected.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (11)
Line range hint
3-15: Imports for new ref-based enhancementsThe import statements for
RefTypeandStringUtilsalign with the changes for ref-based handling. Looks good.
52-53: New request headers for refType and refNameUsing
@RequestHeader(required = false, defaultValue = "branch") RefType refTypeandrefNameimproves flexibility beyond just branch references. This addition is logical and straightforward.
56-57: Conditional handling of refName fallbackThe fallback from
refNametobranchNameensures backward compatibility. This logic is concise and should be safe.
60-65: Ref in logging statementsReplacing
branchNamereferences in the log message withrefTypeandrefNamewill help keep logs consistent with the new naming.
69-69: Passing ref parameters to serviceThe updated method call
.getConsolidatedInfoForPageLoad(...)withrefTypeandrefNameis correct and aligns with the new parameters below.
74-75: Added tags for refType and refNameTagging
refTypeandrefNamein metrics is a good move for analytics.
85-87: New request headers for view mode endpointMirrors the changes in the edit mode endpoint, ensuring consistent handling of
refTypeandrefName.
89-91: Conditional handling of refName fallback (view mode)Same fallback logic as edit mode, ensuring seamless usage for view mode as well. Looks correct.
93-98: Removed direct branch references in log statementEnsures logging remains coherent when dealing with multiple ref types. Good job.
102-102: Service method with new ref parametersConsistently applies the updated signature to the view mode call as well.
108-109: Added tags for refType and refName (view mode)Consistent tagging helps maintain parity with the edit mode endpoint.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (2)
76-76: Switched from getBranchName() to getRefName()This line ensures calls now rely on the updated
RefName, staying consistent with the ref-based model.
144-144: Analytics event uses getRefName()Similarly, switching the event data to
getRefName()completes the transition away from branch-specific logic.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ce/ActionCE_DTO.java (3)
7-7: Import for RefTypeImporting
RefTypealigns with the broader ref-based approach.
176-176: Added refType fieldIntroducing
refTypecaptures the nature of the git reference, no longer limiting the logic to branch references alone.
178-180: New transient refName fieldThe transient
refNameaccompanied by an internal JSON view ensures no unintentional leaking of references to public APIs. This is a sensible addition.app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java (2)
290-290: Refactoring aligns with new reference naming convention
This change frombranchNametorefNameis consistent with the broader refactor approach.
336-336: Consistent usage of refName
UsingsetRefNamehere ensures uniformity and prevents mislabeling branches in test scenarios.app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java (4)
271-271: Use of main refName
Assigning"main"as the refName is clear and maintains consistency across the codebase.
285-285: Feature branch reference
Switching to"feature-branch"helps ensure tests reflect the new refName convention.
316-316: Handling null refName
Settingnullis a valid test case for ensuring robust handling of missing references.
330-330: RefName set to main
Again, maintaining alignment with the updated naming scheme in test coverage.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java (1)
349-350: RefType-RefName update
AdoptingrefTypeandrefNameimproves clarity and aligns with the new reference model.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java (2)
387-387: RefName set for first branch
Shifting from branch-based naming torefNameis consistent with the refactoring goals.
398-398: RefName adjustment for second branch
This ensures multiple branches are distinctly identified with the new reference nomenclature.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
350-351: Ensure null safety forrefTypeandrefName
Consider handling scenarios whennewPagemight have a nullrefTypeorrefNameto avoid potential NullPointerExceptions during runtime.app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (12)
192-192: Review assertion for branch name
Verifying the equality ofbranchandartifactMetadata.getRefName()looks good. Ensure thatartifactMetadata.getRefName()is never null or empty to avoid unexpected behavior.
251-251: Double-check reference alignment
EnsuringartifactMetadata.getRefName()matchesartifactMetadata.getDefaultBranchName()is correct. Confirm thatdefaultBranchNameis always set to the same value to avoid confusion in multi-ref scenarios.
279-279: Branch name assertion
Again, good to see the equality check withartifactMetadata.getRefName(). Keep verifying it’s updated consistently elsewhere in the code.
361-361: Confirm new branch name
fooMetadata.getRefName()is validated against "foo". This test ensures naming consistency. Looks good.
365-365: Logging new commit
Verifying the new commit message is correct. The assertion is straightforward.
392-392: Confirm new branch name
barMetadata.getRefName()is validated against "bar". Similar to the previous branch name check, this ensures correct naming.
396-396: Checking new branch log
Good to see a direct log check for the new branch "bar." This test is valuable for ensuring the commit chain is correct.
420-425: Comprehensive branch listing
Good to see the test verifying local and remote branches (origin/*). This ensures all references are enumerated properly.
448-452: Check updated branch list
Confirming local vs. remote references after branch deletion is correct. Helps validate branch removal logic.
463-468: Branch re-checkout
Ensuring theorigin/foocheckout reintroduces the branch is a valuable test for multi-branch workflows. Looks good.
473-473: Latest commit verification
Verifying the top commit matchestopOfCommitsafter checking out “foo.” This is crucial for ensuring commit history alignment.
581-581: Ensure auto-commit progress retrieval
Looks solid. ThegetAutocommitProgressapproach is clear. Just confirm the behavior doesn't get stuck in unexpected states.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (5)
55-55: Import ofany()from Mockito
Used to match any parameter; usage is standard. Ensure the test covers correct argument references to avoid false positives.
204-205: Verify method call parameters
This mocking ofnewPageService.findByRefTypeAndRefNameAndBasePageId(...)checks for multiple arguments. Ensure eachany()usage corresponds to correct parameter types.
238-239: Reference-based page lookup
Again, the same method is mocked. This approach is consistent with the new naming convention for references.
296-297: Consistency in ref-based methods
Your test aligns well with the changed method signature. Keep verifying no older methods remain in the code.
392-393: Mock for ref-based retrieval
The test extends coverage for scenarios where page references differ. Looks adequate.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (3)
364-364: Update existing action
A clean approach to callingupdateExistingAction. Ensure concurrency issues don’t arise if multiple threads import the same action.
509-510: Permission check
Ensuring the correct edit permission for the existingAction is crucial. Good job throwing an error if the user lacks ACL.
521-523: Ref type & ref name updates
This block properly setsexistingAction.setRefTypeandexistingAction.setRefName. Confirm thatimportingMetaDTO.getRefType()and.getRefName()are never null.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
299-299: Consolidate reference handling code commentGreat job updating the condition to use
RefType.branch. If you anticipate supporting additional reference types, consider a switch statement for better extensibility.
505-505: Confirm presence of the correct referenceWhen checking out a branch by
refName, a potential pitfall is referencing a remote branch unintentionally. A helpful step might be to validate whether the provided ref is local or remote if that matters for your logic.app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (1)
271-272: Keep or remove reference fields intentionallySetting both
refTypeandrefNameto null here resets the Git-associated references. Confirm this aligns with your overall design or consider leaving them intact if the fork might preserve partial Git context.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (1)
311-314: Improve logic for identifying branched appsThe conditional block now checks
gitData.getRefName()anddefaultArtifactId. This is good for clarity. If you ever return to a multi-branch approach, ensure consistent usage of default references.app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java (5)
216-216: Use descriptive reference names in testing
gitData.setRefName("actionServiceTest")is perfectly fine. Just be mindful of naming across multiple test cases so test logs are self-explanatory.
224-224: Add consistency in artifact callsPassing
gitData.getRefName()for artifact export is correct. Double-check if there are more references tobranchNamelingering in test setup or teardown.
229-229: Check for uniformity in repeated callsThe logic for importing artifacts from Git matches the new reference approach. If multiple branches are tested, ensure they follow the same pattern.
1106-1107: Retain clarity around newAction propertiesSetting
newAction1.setRefTypeandnewAction1.setRefNameensures the new action is fully in sync with the updated Git references. Confirm that each usage path updates or clears these as intended.
1193-1194: Uniform approach for referencingApplying the same reference assignment logic here avoids inconsistencies. Keep watch for any test flakiness if
refTypeandrefNameget overwritten in subsequent steps.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (2)
399-402: Ensure null-safe usage ofpagefields
Ifpage.getRefType()orpage.getRefName()might be null, consider handling null scenarios to avoid potential issues.
529-531: Honor environment-specific reference name
These lines correctly apply the branched application’srefName. Just be sure to confirm that any potential null or empty values forrefNameare handled elsewhere if needed.app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
382-382: Enum constant rename looks consistent
ReplacingTAG.equalswithtag.equalsaligns with the newly adopted lowercase enum naming convention.app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java (2)
1004-1004: Renaming branchName to refName
UpdatinggitArtifactMetadata.setRefName("master")is consistent with the ref-focused approach.
1026-1026: Consistency in Git metadata field naming
UsinggitArtifactMetadata1.setRefName("feature1")aligns with the shift torefName.app/server/appsmith-server/src/test/java/com/appsmith/server/exports/internal/ExportServiceTests.java (4)
834-834: Adoption ofrefNameingitData
Good use of the updated property to reflect the new reference naming scheme.
942-942: Switch frombranchNametorefName
Ensures clarity by describing the actual reference type (branch, tag, etc.).
1426-1426: Inherits cohesive naming
This line supports the unified approach for referencing Git metadata.
1485-1485: ConsistentrefNameusage
Reiterates the standardized naming convention for Git references.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (20)
211-211: Consider verifying the default branch isn't null.
A quick validation check can safeguard us when callingsetRefName(defaultBranch).
239-239: LGTM
UsingRefType.branchclarifies the intended reference type.
490-490: Ensure baseRefName is non-null before usage.
A quick null-check helps prevent errors if metadata is missing.
503-503: Condensed condition check is fine
The logic to compare old vs. new references looks good.
697-697: LGTM
Switching toRefType.branchis consistent with the overall ref-based renaming.
1009-1009: Consider ensuringdefaultBranchis set earlier
This line functions well but verifying that thedefaultBranchis properly derived can avoid confusion.
Line range hint
1191-1191: Clarify boilerplate vs. hardcoded string
Here,"defaultBranch"is a literal assignment. Confirm if this is intended or a placeholder.
Line range hint
1228-1228: Explicitly settingRefType.branch
This is a clear approach indicating we are dealing with branches, not tags.
1285-1285: Implementation meets the approach
UsingjsonMorphDTO.setRefType(RefType.branch)is consistent with new naming.
1292-1292: Watch for non-nullrefName
Same pattern: good to ensurebranchedArtifactMetadata.getRefName()isn’t null before using.
1301-1301: Keeps usage consistent
RepeatingRefType.branchusage is aligned with the new naming standard.
1306-1306: Same note aboutgetRefName()
Ensure it’s not null to avoid unforeseen NullPointer issues.
1352-1352: No issues
Reading the ref name is straightforward here.
1505-1505: Synchronized naming
Keeping them all as final references is good for clarity.
1522-1522: Consistent approach
The code usesbranchin the correct place again.
1536-1536: Matches therefTypeapproach
This helps unify all references under theRefType.branchconcept.
1651-1651: Consider local var usage
CapturingbranchedGitMetadata.getRefName()in a local variable is clean.
1695-1695: Good code clarity
Consistent with earlier usage.
1699-1699: ClearRefTypeusage
No issues, the code is uniform.
Line range hint
1780-1780: Use the new refName
Ensures merges reference the correct field.app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (12)
7-7: ImportingRefType
This aligns with the updated enumeration.
694-694: Validation
Nice assertion that ensures therefNameis as expected.
773-773: Same validation
Confirms the renamed property is set.
884-884: ChecksrefName
Keeping tests strongly consistent with new fields is a plus.
1091-1091: No concerns
Straightforward test assertion forrefName.
1228-1228: Test clarity
Usingaction.setRefType(RefType.branch);is an effective test scenario.
1732-1732: Asserting correct usage
Ensures the merge logic uses the newrefName.
1780-1780: Mirror usage
Again verifyinggetRefName()in merges.
3852-3852: Consider test scenario clarifications
SettingrefName("not-present-branch")helps confirm error handling.
4072-4072: Great negative test
doesNotContain(TO_BE_DELETED_BRANCH)ensures correct deletion outcome.
4194-4194: RefName check
Asserting the new property modifies the correct reference.
4228-4228: Confirming post-checkout
Verifying the final state’srefNameis correct.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (11)
4-4: New Import forRefTypeLooks Good
No issues found with the newly introduced import statement.
333-333: Confirming use ofRefType.branch
SpecifyingRefType.branchin test seems appropriate for the unpublished actions call.
1370-1370: SettinggitData.setRefName("testBranch")
This line is consistent with the test objective to verify branch-based importing.
1403-1403: Importing Artifact with Branch Name
CallingimportArtifactInWorkspaceFromGitwith the ref name is correct here.
1422-1422: RetrievingrefNamefor Verification
Usage ofapplication.getGitApplicationMetadata().getRefName()aligns with verifying updated references.
3298-3298: Importing withgitData.getRefName()
This usage is consistent with prior tests for verifying navigation settings behavior.
3338-3338: Setting Branch Name to"testBranch"
This is an appropriate step to validate the behavior with a custom branch.
3356-3356: Importing Resource with Custom Branch
No issues with leveraging the provided ref name as part of the import flow.
3958-3958: Using"feature"for New Branch
Choosing"feature"is fine for verifying separate branch import logic.
4069-4069: Assigning"feature"togitData1.setRefName
No issues found with selecting"feature"as the branch for test coverage.
4180-4180: Another"feature"Branch Reference
All good here for verifying multi-branch scenarios.app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (16)
241-241: Confirmed safe usage after validation.
We've already validated therefName, so there's no risk of passing a null or empty string tocheckoutToBranch.
275-275: Final local variable usage.
StoringrefNameinfinalBranchNameis consistent with the new naming. No issues found.
396-396: Adequate null and emptiness checks.
EnsuresbranchedGitDataandrefNamearen't null or empty, preventing unexpected runtime errors.
400-400: Clear reference assignment.
UsingbranchedArtifact.getGitArtifactMetadata().getRefName()is in line with the new refName pattern.
992-992: Consistent usage of new naming.
CheckingbranchedGitMetadata.getRefName()aligns well with the refName approach.
1210-1210: Ensure prior validation.
checkoutToBranchnow uses the refName. Confirm it's validated before use to avoid potential null issues.
1217-1217: Push with refName.
This usage is consistent with the updated refName references.
1303-1303: Careful with hard resets.
Ensure that discarding local changes is intended. The usage here is correct withrefName.
1308-1308: Helpful context in error message.
IncludinggitMetadata.getRefName()in the message helps users identify the problematic branch reference quickly.
1692-1692: Concise commit message usage.
AppendingbranchedGitMetadata.getRefName()to the commit message is consistent with the new naming.
2061-2061: No issues with ternary usage.
RefName is retrieved correctly in the conditional expression.
2149-2149: Final variable usage.
AssigningbranchedGitMetadata.getRefName()tofinalBranchNameis consistent with the updated naming scheme.
2172-2172: Consistent usage of refName.
The final variable approach fosters clarity when referencingrefName.
2343-2343: RefName equality check.
This condition correctly usesgetRefName()for comparison withdestinationBranch.
2527-2527: Duplicate logic usage.
Matches earlier checks fordestinationBranchvs.getRefName(). Implementation looks good.
2909-2909: Conditional check on refName.
ComparingbranchNametogetRefName()is logically correct for the refName-based approach.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (1)
515-516: Check for null values before assigning references.
To avoid potentialNullPointerExceptions, consider verifying thatactionCollection.getRefType()andactionCollection.getRefName()are non-null before assigning them tonewAction.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (13)
4-4: Reference type import addition
The addition ofRefTypelooks consistent with the broader ref-based approach implemented across the codebase.
142-142: Method signature update
Ensure all invocations correctly passrefTypeandrefNameto align with the new signature.
153-153: Updated method call
ForwardingrefTypeandrefNametogetAllFetchableMonosappears correct.
162-163: Method signature alignment
IntroducingRefType refType, String refNamekeeps method usage consistent with the new reference approach.
227-227: Parameter expansion
Good job passingrefTypeandrefNamehere to retrieve matching application/page data.
504-505: Method signature
The parametersRefType refType, String refNameare now mandatory. Verify all calling locations handle these new arguments.
515-515: Ref-based lookup
Ensure thenewPageService.findByRefTypeAndRefNameAndBasePageIdAndApplicationModeaccurately handles null or missing references.
521-521: RefName parameter usage
No issues found; updating the call to includerefNameis straightforward.
559-560: Potential null arguments
CallingfindByRefTypeAndRefNameAndBasePageIdAndApplicationMode(null, null, ...)suggests ensuring the method can handle null safely.
563-563: Conditional check
CheckingbasePage.getRefName()is fine. No concerns here.
577-577: No issues found
The log message suits the scenario of a non–git-connected application.
598-598: Default branch name comparison
Ensure exact (case-sensitive) matching betweendefaultBranchNameandrefNameis intended.
626-626: Hard-coded refType usage
UsingRefType.branchis logical for retrieving the default branch data here.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (7)
5-5: RefType import
The new import forRefTypealigns correctly with the code changes for branch-to-ref conversions.
173-174: Non-null reference checks
Before assigningnewAction.getRefType()andnewAction.getRefName()toaction, consider gracefully handling null or unset values.
198-199: Field mirroring
Assigningaction.getRefType()andaction.getRefName()tonewActionlooks consistent. No immediate issues noted.
988-988: Signature expansion
Keep in mind to update or create unit tests to cover scenarios with differentrefTypeandrefNamevalues.
994-995: New method usage
Referencing the newly introducedfindByRefTypeAndRefNameAndBasePageIdmethod ensures correct retrieval by reference.
999-999: Application fetch
Fetching application viarefNameand base ID is coherent with the codebase’s ref-based model.
1030-1032: Updated method signature
Double-check existing callers passRefTypeandrefNameproperly to avoid runtime errors.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Context.java (3)
3-4: RefType import addition
This import is appropriate for defining the reference type in the interface.
13-14: New method definition
AddingRefType getRefType()suits the approach for identifying references by type.
15-15: Reference name getter
String getRefName()effectively replaces the old branch-based getter. Good job!app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ClonePageMetaDTO.java (2)
3-3: Import statement
Bringing inRefTypeis consistent with the ref-based modifications across the codebase.
19-20: Added fields
refTypeandrefNameneatly replace the singlebranchName, providing clearer reference context.
| actionCollection.setRefType(null); | ||
| actionCollection.setRefName(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate repetitive blocks.
You're setting refType and refName to null multiple times (pages, actions, and action collections). Extract these steps into a helper function (e.g. clearRefProperties(object)) to reduce code duplication.
- newPage.setRefType(null);
- newPage.setRefName(null);
- newAction.setRefType(null);
- newAction.setRefName(null);
- actionCollection.setRefType(null);
- actionCollection.setRefName(null);
+ clearRefProperties(newPage);
+ clearRefProperties(newAction);
+ clearRefProperties(actionCollection);Committable suggestion skipped: line range outside the PR's diff.
| .block(); | ||
|
|
||
| branchName = gitConnectedApp.getGitApplicationMetadata().getBranchName(); | ||
| branchName = gitConnectedApp.getGitApplicationMetadata().getRefName(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename the branchName variable to refName.
Assigning gitConnectedApp.getGitApplicationMetadata().getRefName() to a variable still called branchName could cause confusion.
| GitArtifactMetadata gitData = application.getGitApplicationMetadata(); | ||
| if (gitData != null) { | ||
| page.setBranchName(gitData.getBranchName()); | ||
| page.setRefName(gitData.getRefName()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include refType for consistency when setting refName
You are setting only the new page’s refName from the gitApplicationMetadata but not the refType. If the application is linked to a tag or another ref type, this mismatch may cause inconsistency.
if (gitData != null) {
+ page.setRefType(gitData.getRefType());
page.setRefName(gitData.getRefName());
}📝 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.
| page.setRefName(gitData.getRefName()); | |
| if (gitData != null) { | |
| page.setRefType(gitData.getRefType()); | |
| page.setRefName(gitData.getRefName()); | |
| } |
2 participants
WIP
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.All" it=true
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12573592991
Commit: 5e2ad53
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 01 Jan 2025 21:24:51 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes for end-users:
Git Reference Management
Application and Resource Handling
API and Service Updates
These changes represent a significant refactoring of the Git and version control infrastructure, providing more flexibility and clarity in how resources are tracked and referenced.