feat: branching #3246
feat: branching #3246
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 36 files out of 143 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughAdds project branch support across backend, database, and frontend: new Branch entities and migrations, branch-aware key/translation queries and services, branch merge/dry-run/conflict workflows with APIs and UI for branch management and selection. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Frontend
participant API as BranchController / TranslationsController / KeyController
participant Service as BranchService / KeyService / TranslationService
participant Repo as Repository Layer
participant DB as Database
User->>UI: open translations or branches view
UI->>API: GET /v2/projects/{id}/branches or GET keys/translations?branch={name}
API->>Service: fetch branches or branch-scoped keys/translations (branch param)
Service->>Repo: execute branch-aware query (LEFT JOIN branch, filter archived/default)
Repo->>DB: SELECT ... FROM key LEFT JOIN branch ... WHERE (branch IS NULL OR branch.name = :branch ...)
DB-->>Repo: rows
Repo-->>Service: mapped DTOs (KeyView/Translation)
Service-->>API: return models
API-->>UI: render branch-aware list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
dkrizan
force-pushed
the
danielkrizan/branching
branch
3 times, most recently
from
78c037f to
6371745
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/LanguageStatsProvider.kt (1)
80-101: Critical: Subquery lacks project correlation.Same issue as in
selectKeyCount- the subquery aggregates across all projects instead of correlating with the outer query's current project row.Apply this diff to correlate the subquery with the outer query's project:
private fun selectWordCount(state: TranslationState): Selection<Int> { val sub = query.subquery(Int::class.java) val project = sub.from(Project::class.java) val keyJoin = project.join(Project_.keys) joinDefaultBranch(sub, keyJoin) joinTargetTranslations(keyJoin, state) + sub.where( + cb.and( + sub.restriction, + cb.equal(project.get(Project_.id), this.project.get(Project_.id)) + ) + ) + val baseTranslationJoin = keyJoin.join(Key_.translations, JoinType.LEFT).also { translation -> translation.on( cb.equal(translation.get(Translation_.language), project.get(Project_.baseLanguage)), ) } val count = cb.sum(baseTranslationJoin.get(Translation_.wordCount)) val coalesceCount = cb.coalesce<Int>() coalesceCount.value(count) coalesceCount.value(0) return sub.select(coalesceCount) }Note: Use
this.projectto disambiguate from the localprojectvariable.backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/ProjectStatsProvider.kt (1)
100-101: Bug: counting task IDs using Tag_.idCounting tasks should use Task_.id, not Tag_.id.
- return sub.select(cb.coalesce(cb.countDistinct(task.get(Tag_.id)), 0)) + return sub.select(cb.coalesce(cb.countDistinct(task.get(Task_.id)), 0))backend/data/src/main/kotlin/io/tolgee/dtos/IExportParams.kt (1)
109-125: filterBranch is not copied in copyPropsFrom()copyPropsFrom omits the new filterBranch, leading to lost branch filters when cloning params.
Apply:
fun copyPropsFrom(other: IExportParams) { this.languages = other.languages this.format = other.format this.structureDelimiter = other.structureDelimiter this.filterKeyId = other.filterKeyId this.filterKeyIdNot = other.filterKeyIdNot this.filterTag = other.filterTag this.filterTagIn = other.filterTagIn this.filterTagNotIn = other.filterTagNotIn this.filterKeyPrefix = other.filterKeyPrefix this.filterState = other.filterState this.filterNamespace = other.filterNamespace this.messageFormat = other.messageFormat this.supportArrays = other.supportArrays this.fileStructureTemplate = other.fileStructureTemplate this.escapeHtml = other.escapeHtml + this.filterBranch = other.filterBranch }
🧹 Nitpick comments (42)
webapp/src/eeSetup/eeModule.ee.tsx (1)
216-218: Consider guarding with PrivateRoute (align with auth gating)If other Project subroutes require auth, wrap BranchesView with PrivateRoute for consistency.
- <Route path={LINKS.PROJECT_BRANCHES.template}> - <BranchesView /> - </Route> + <PrivateRoute path={LINKS.PROJECT_BRANCHES.template}> + <BranchesView /> + </PrivateRoute>backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (1)
10-10: Make originBranchId optional for flexibilityCreation may not always have/require an origin branch (e.g., empty branch, default base). Consider nullable with default.
- fun createBranch(projectId: Long, name: String, originBranchId: Long): Branch + fun createBranch(projectId: Long, name: String, originBranchId: Long? = null): Branchbackend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt (1)
350-356: Redundant author assignment in addCodeReferenceYou pass author to addCodeReference and also set author inside the lambda. Keep one; prefer the constructor arg for clarity. Same pattern appears further below (Lines 375–382, 389–395).
- addCodeReference(userAccount) { + addCodeReference(userAccount) { path = "./code/exist.extension" line = 10 fromImport = true - author = userAccount }webapp/src/component/branching/BranchLabel.tsx (1)
12-15: Type the ref target and add title for truncated content
- Add generics to forwardRef for correct ref typing.
- Provide title to show full branch name on hover.
-export const BranchLabel = React.forwardRef(function BranchLabel( - { value, placeholder, ...rest }: InputBaseComponentProps, - ref -) { +export const BranchLabel = React.forwardRef<HTMLDivElement, InputBaseComponentProps>(function BranchLabel( + { value, placeholder, ...rest }, + ref +) { return ( <StyledFakeInput tabIndex={0} ref={ref} - style={{ display: 'flex' }} + style={{ display: 'flex' }} {...(rest as any)} > <GitBranch02 height={20} width={20} style={{ marginRight: 10 }} /> - <div>{value || placeholder}</div> + <div title={String(value ?? placeholder ?? '')}>{value || placeholder}</div> </StyledFakeInput> ); });Also applies to: 17-25
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/ProjectStatsProvider.kt (1)
56-65: Scope default-branch filter to the same project (defensive)Current predicate counts keys where branch is null or branch.isDefault = true. Add a project match to avoid accidental cross-project linkage if data ever gets inconsistent.
sub.where( cb.and( key.get(Key_.project) equal project, cb.or( cb.isNull(key.get(Key_.branch)), cb.and( cb.isTrue(branch.get(Branch_.isDefault)), branch.get(Branch_.project) equal project ) ) ) )webapp/src/component/searchSelect/InfiniteSearchSelect.tsx (1)
50-55: Tighten types for inputComponent and menuAnchorOrigin
- inputComponent: allow intrinsic string types too by using React.ElementType.
- menuAnchorOrigin: use MUI’s PopoverOrigin (or MenuProps['anchorOrigin']) to stay aligned with upstream types.
- inputComponent?: React.ComponentType<InputBaseComponentProps>; - menuAnchorOrigin?: { - vertical: 'top' | 'bottom'; - horizontal: 'left' | 'right'; - }; + inputComponent?: React.ElementType<InputBaseComponentProps>; + menuAnchorOrigin?: import('@mui/material').PopoverOrigin;Usage remains the same:
- inputComponent: inputComponent || FakeInput, + inputComponent: inputComponent || FakeInput,Default stays:
anchorOrigin={menuAnchorOrigin || { vertical: 'top', horizontal: 'left' }}Also applies to: 75-77, 131-131, 166-171
backend/data/src/main/resources/db/changelog/schema.xml (1)
4671-4675: Remove transient UC creation/drop if possible (noise/fragility)You create UC_BRANCHORIGIN_BRANCH_ID_COL and later drop it, and also replace the (project_id, name) UC with a partial unique index. If this PR hasn’t shipped to any env, prefer squashing to the final state to avoid unnecessary create/drop churn.
If some environments already applied earlier changeSets, keep the drop to ensure forward-only migrations.
Also applies to: 4724-4729
backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/TranslationFilters.kt (1)
161-164: Consider naming consistency.The field is correctly implemented with proper documentation. However, note that
ContentDeliveryConfigRequestusesfilterBranchwhile this class usesbranch. While both work correctly, consistent naming (e.g.,filterBranch) would align with other filter properties likefilterKeyId,filterTag, etc.backend/data/src/main/kotlin/io/tolgee/dtos/request/key/CreateKeyDto.kt (1)
51-51: Add @Schema annotation for API documentation.For consistency with other fields in this DTO and to provide clear API documentation, consider adding a
@Schemaannotation describing the branch parameter.Apply this diff:
+ @Schema(description = "The branch of the key. (When empty or null default branch will be used)") val branch: String? = null,webapp/src/component/branching/BranchSelector.tsx (1)
34-38: Consider simpler layout.The
Boxcomponent withdisplay="grid"seems unnecessary for a single child element. Unless this is preparing for future layout additions, a simpler approach would suffice.Apply this diff:
return ( - <Box display="grid"> + <Box> <BranchSelect branch={selected} onSelect={changeBranch} /> </Box> );Alternatively, you could remove the Box entirely:
return ( - <Box display="grid"> - <BranchSelect branch={selected} onSelect={changeBranch} /> - </Box> + <BranchSelect branch={selected} onSelect={changeBranch} /> );backend/data/src/main/kotlin/io/tolgee/dtos/request/key/ComplexEditKeyDto.kt (1)
63-64: LGTM! Consider adding validation for branch name format.The new optional
branchfield is well-documented and follows the pattern of other optional parameters in the DTO. However, consider whether branch names should have format validation (e.g., length constraints, allowed characters) to prevent invalid branch names from being accepted at the API layer.backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/SetTranslationsWithKeyDto.kt (1)
38-39: LGTM! Consider consistent wording across DTOs.The new
branchfield is properly documented. For consistency, consider aligning the description with other DTOs (e.g., ComplexEditKeyDto mentions "If not provided, default branch will be used") to provide uniform API documentation across branch-aware endpoints.backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
267-284: Consider extracting branch filtering logic to reduce duplication.The branch filtering logic is nearly identical to the implementation in
ExportDataProvider(lines 157-173). Both handle null/default branch fallback and archived branch exclusion.Consider extracting this pattern into a shared utility method or extension function:
// Example utility fun CriteriaBuilder.branchFilter( branchJoin: Join<Key, Branch>, branchName: String? ): Predicate { return if (branchName.isNullOrEmpty()) { or( branchJoin.get(Branch_.id).isNull, isTrue(branchJoin.get(Branch_.isDefault)) ) } else { and( equal(branchJoin.get(Branch_.name), literal(branchName)), isNull(branchJoin.get(Branch_.archivedAt)) ) } }Then both locations could use:
whereConditions.add(cb.branchFilter(branchJoin, params.branch))This would make the logic more maintainable and reduce the risk of inconsistencies if the filtering rules change.
backend/data/src/main/kotlin/io/tolgee/service/bigMeta/BigMetaService.kt (1)
233-242: Add clarifying comment for complex branch filtering logic.The nested OR predicate at lines 233-242 handles branch matching for key similarity detection, but its purpose may not be immediately clear.
Consider adding a comment explaining the intent:
cb.or( // Match keys with specified branch (if not archived) cb.and( cb.equalNullable(branch.get(Branch_.name), key.branch), cb.isNull(branch.get(Branch_.archivedAt)), ), // Also match keys with no branch (legacy) or default branch cb.or( cb.isNull(root.get(Key_.branch)), cb.isTrue(branch.get(Branch_.isDefault)), ) )This clarifies that the logic includes keys that:
- Match the specified branch (when not archived), OR
- Have no branch association (backward compatibility), OR
- Are on the default branch
The complexity is justified here since BigMeta needs to find similar keys across branch contexts, but the comment would help future maintainers.
webapp/src/component/branching/BranchSelectField.tsx (1)
5-5: Improve type safety for Formik context.The
useFormikContexthook is typed asany, which eliminates type checking and could hide errors.Apply this diff to improve type safety:
- const { setFieldValue } = useFormikContext<any>(); + const { setFieldValue } = useFormikContext();Alternatively, if you need to constrain the form values, define a proper interface:
interface BranchFormValues { name: string; originBranchId: number; } const { setFieldValue } = useFormikContext<BranchFormValues>();backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTranslationsTestData.kt (1)
43-50: Consider using a builder pattern for tag creation.Tags are created via direct
Tag()instantiation, while other entities (labels, branches, languages) use the builder pattern. For consistency and to align with the test data builder conventions used elsewhere, consider adding tags through a builder method if one exists (e.g.,addTag { ... }).backend/app/src/test/kotlin/io/tolgee/service/TranslationServiceTest.kt (1)
21-26: Prefer setOf(...) over HashSet(listOf(...))Kotlin-idiomatic and clearer. No behavior change.
- languageTags = HashSet(listOf("en", "de")), + languageTags = setOf("en", "de"),Apply to all getTranslations calls above.
Also applies to: 39-44, 90-95, 107-112, 124-129
webapp/src/views/projects/translations/context/services/useEditService.tsx (1)
80-96: Only send branch when explicitly selected via routeUsing selected?.name may send a placeholder default object’s name; prefer the route-derived selectedName.
- branch: branchesService.selected?.name, + branch: branchesService.selectedName ?? undefined,webapp/src/ee/branching/components/BranchItem.tsx (1)
64-71: Improve a11y and test selector
- Add aria-label to delete button.
- Fix data-cy typo “branchs” → “branches”.
- <IconButton - data-cy="project-settings-branchs-remove-button" + <IconButton + aria-label="Delete branch" + data-cy="project-settings-branches-remove-button" size="small" onClick={() => onRemove(branch)} >webapp/src/ee/branching/components/BranchesList.tsx (2)
110-128: Fix test id and empty state text; guard protected deletion
- data-cy says labels; should be branches.
- Empty placeholder uses labels i18n key; switch to branches key.
- Prevent delete for protected branches too.
- <PaginatedHateoasList + <PaginatedHateoasList loadable={branchesLoadable} onPageChange={setPage} listComponent={TableGrid} - data-cy="project-settings-labels-list" + data-cy="project-settings-branches-list" emptyPlaceholder={ <Box m={2} display="flex" justifyContent="center"> <Typography color="textSecondary"> - {t('project_settings_no_labels_yet')} + {t('project_settings_no_branches_yet')} </Typography> </Box> } renderItem={(l: BranchModel) => ( <BranchItem branch={l} - onRemove={!l.isDefault ? deleteBranch : undefined} + onRemove={!l.isDefault && !l.isProtected ? deleteBranch : undefined} /> )} />Please confirm the i18n key exists or provide the correct one.
97-107: Wire edit permission when availableReplace hardcoded canEditBranches with permission check (as your TODO hints).
- const canEditBranches = true; // TODO satisfiesPermission('branches.edit') + const canEditBranches = satisfiesPermission?.('branches.edit') ?? true;webapp/src/component/branching/BranchSelect.tsx (1)
73-91: Optional: Wire search input to backend for large project branch lists.You pass
search={''}and never fetch more. If branch counts grow, allow live search and pagination.Minimal sketch:
+ const [search, setSearch] = useState(''); return ( <InfiniteSearchSelect items={branches} queryResult={loadable} itemKey={(item) => item.id} selected={selected} minHeight={false} - search={''} + search={search} + onSearchChange={setSearch} + onFetchMore={() => loadable.fetchNextPage?.()} renderItem={renderItem}Coordinate with API to thread a
searchquery to/branches.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt (2)
24-37: Validate branch-project consistency early.Fail fast if branches don’t belong to
projectId; otherwise copy becomes a silent no-op.Example:
@Transactional override fun copy(projectId: Long, sourceBranch: Branch, targetBranch: Branch) { require(sourceBranch.id != targetBranch.id) { "Source and target branch must differ" } + require(sourceBranch.project.id == projectId && targetBranch.project.id == projectId) { + "Branches must belong to the provided project" + }
52-55: Make namespace join sargable; avoidcoalesce()on indexed columns.
coalesce(tk.namespace_id,0) = coalesce(sk.namespace_id,0)inhibits index use. Prefer null-safe equality that’s index-friendly in Postgres.Postgres-specific alternative:
-join target_keys tk on tk.name = sk.name and coalesce(tk.namespace_id,0) = coalesce(sk.namespace_id,0) +join target_keys tk + on tk.name = sk.name + and ((tk.namespace_id = sk.namespace_id) or (tk.namespace_id is null and sk.namespace_id is null))Apply similarly in
copyKeys,copyTranslations,copyKeyMetas,copyKeyMeta*,copyTranslation*.Also applies to: 90-93, 136-140, 184-187, 231-234, 263-271, 338-341, 371-375
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (2)
31-36: Prefer centralized CORS config over controller-level wildcard
@CrossOrigin(origins = ["*"])weakens security guarantees. Move CORS to a global config (allowlisted origins per env) and drop the annotation here.
45-58: List permission can be read-onlyConsider relaxing GET to a read scope (e.g., KEYS_VIEW) to align with least privilege. POST/DELETE can keep KEYS_EDIT.
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (2)
32-39: Infinite query not needed for a single, large pageYou request
size: 10000and don’t providegetNextPageParam. Use a simple query hook or setgetNextPageParamif multiple pages are expected.
18-24: Placeholder default branch with id 0 risks invalid writesIf UI submits before real data loads,
originBranchId(or consumers) may get 0. Prefer leaving branches empty until loaded or mark default only for display (never propagate its id).Consider:
- returning
branches: []untilisSuccess, and- keeping
defaultBranchseparate for UI hinting.webapp/src/ee/branching/components/BranchForm.tsx (1)
53-58: Ensure BranchSelect sets a real branch id before submitConsider disabling submit until
originBranchIdis truthy, or have BranchSelect emit only when real data is loaded. If needed, passdefaultBranchdown to avoid placeholder ids.ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt (1)
55-69: Also assert 201 Created on branch creationOnce the controller returns 201, assert status to lock API semantics.
- ).andAssertThatJson { + ).andIsCreated.andAssertThatJson {ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (4)
45-47: Redundant same‑project check
findByProjectIdAndId(projectId, originBranchId)already guarantees project ownership. The extra check is dead code.Apply:
- if (originBranch.project.id != projectId) { - throw BadRequestException(Message.ORIGIN_BRANCH_NOT_FOUND) - }
53-56: Ensure new branch ID is generated before copyIf
Branch.iduses IDENTITY/DB‑generated values,save()may not flush immediately. IfbranchCopyService.copy(...)relies onbranch.id, add a flush.branchRepository.save(branch) + entityManager.flush() // ensure branch.id is available for the copy branchCopyService.copy(projectId, originBranch, branch)
75-81: Publish deletion event after commitPublishing inside the open transaction risks listeners observing stale state. Prefer after‑commit publication or transactional event listeners.
Option A (publish after commit):
+import org.springframework.transaction.support.TransactionSynchronization +import org.springframework.transaction.support.TransactionSynchronizationManager ... - applicationContext.publishEvent(OnBranchDeleted(branch)) + TransactionSynchronizationManager.registerSynchronization(object : TransactionSynchronization { + override fun afterCommit() { + applicationContext.publishEvent(OnBranchDeleted(branch)) + } + })Option B: switch listeners to
@TransactionalEventListener(phase = AFTER_COMMIT)and keep current publish call.
63-73: Helper should be private; clarify name reuse vs. archived; add DB uniqueness
- Make this helper private to avoid unintended external use.
- The name collision check doesn’t filter archived branches. Decide if names can be reused after archival and adjust the repo method accordingly.
- Guarding by read‑then‑write is not atomic; add a DB unique constraint (e.g., partial unique index on (project_id, lower(name)) where archived_at is null) and map DataIntegrityViolation to BRANCH_ALREADY_EXISTS.
- fun createBranch(projectId: Long, name: String): Branch { + private fun createBranch(projectId: Long, name: String): Branch {ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (2)
78-84: Avoid hard timing assertions in CI
time.assert.isLessThan(3000)is environment‑dependent and flaky. Use a higher bound, conditional skip on CI, or assert functional readiness instead (e.g., wait forpending=false).
118-186: Tighten assertions and naming for readability
- Variable names
main*/branch*are swapped relative to actual objects; rename tocopy*/origin*.- Guard
.first()calls on comments/screenshots with presence checks to avoid brittle failures if data changes.backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt (2)
99-127: Search excludes non‑default branches
searchKeysfilters(br.id is null or br.is_default)and has no branch parameter. With branching, consider adding an optionalbrancharg to search within a branch (and default/null fallback).
60-71: Alias/style consistency (minor)You join
k.branch brbut selectk.branch.name. Preferbr.namefor clarity and to avoid accidental extra joins.- select new io.tolgee.dtos.queryResults.KeyView(k.id, k.name, ns.name, km.description, km.custom, k.branch.name) + select new io.tolgee.dtos.queryResults.KeyView(k.id, k.name, ns.name, km.description, km.custom, br.name)backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (2)
216-221: Branch resolution via collection scan
project.branches.find { ... }can trigger a lazy load and is O(n). Prefer repository lookup to avoid loading the whole collection.If feasible in this layer, use a
BranchRepository.findByProjectIdAndName(project.id, branch); otherwise, cache/default-branch accessor to avoid multiple loads.
200-202: Uniqueness check is non‑atomic
checkKeyNotExisting+ insert is prone to races. Ensure a DB unique constraint covers (project_id, namespace_id, branch_id, lower(name)) with appropriate null handling for default branch, and translate integrity violations toKEY_EXISTS.webapp/src/service/apiSchema.generated.ts (2)
1671-1677: Inconsistent param naming: filterBranch vs branch.Export endpoints use filterBranch; key/translation queries use branch. Consider aligning naming (e.g., filterBranch everywhere alongside other filter* params) while still in draft to avoid future breaking changes.
Also applies to: 1797-1803, 2558-2563, 12392-12397, 18307-18309, 13415-13416, 13851-13853, 17939-17941
413-419: Use descriptive operationIds for branch endpoints
Operations["all"], create_11, and delete_15 appear only inapiSchema.generated.ts; no client code references them. Rename these in the OpenAPI spec to listBranches, createBranch, deleteBranch, then regenerate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (103)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/keys/KeyController.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/TranslationsController.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/component/KeyComplexEditHelper.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/controllers/ExportController.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/contentDelivery/ContentDeliveryConfigModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModelAssembler.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithDataModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithDataModelAssembler.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithScreenshotsModelAssembler.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/AllKeysControllerTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerModificationTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ImportController/SingleStepImportControllerTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerCreationTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerKeySearchTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerTest.kt(4 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ProjectsController/ProjectsControllerCreateTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/service/LanguageServiceTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/service/TranslationServiceTest.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/constants/Message.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt(5 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchBuilder.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTranslationsTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/ExportParamsDocs.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/IExportParams.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/RelatedKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/KeyIdFindResult.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/KeyView.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/ContentDeliveryConfigRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/export/ExportParams.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/ComplexEditKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/CreateKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/EditKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/SetTranslationsWithKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/TranslationFilters.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/importKeysResolvable/ImportKeysResolvableItemDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/events/OnBranchDeleted.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/Project.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/Branch.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/repository/KeyMetaRepository.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt(9 hunks)backend/data/src/main/kotlin/io/tolgee/repository/TranslationRepository.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/bigMeta/BigMetaService.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/bigMeta/KeysDistanceUtil.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchCopyService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt(5 hunks)backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt(6 hunks)backend/data/src/main/kotlin/io/tolgee/service/key/ResolvingKeyImporter.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/project/ProjectCreationService.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/LanguageStatsProvider.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/ProjectStatsProvider.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/translation/TranslationService.kt(2 hunks)backend/data/src/main/resources/db/changelog/schema.xml(1 hunks)backend/data/src/test/kotlin/io/tolgee/unit/bigMeta/KeysDistanceUnitTestData.kt(1 hunks)e2e/cypress/support/dataCyType.d.ts(2 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/BranchModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/CreateBranchModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/BranchRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCleanupService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)webapp/src/component/branching/BranchLabel.tsx(1 hunks)webapp/src/component/branching/BranchSelect.tsx(1 hunks)webapp/src/component/branching/BranchSelectField.tsx(1 hunks)webapp/src/component/branching/BranchSelector.tsx(1 hunks)webapp/src/component/branching/DefaultBranchChip.tsx(1 hunks)webapp/src/component/common/ClipboardCopy.tsx(1 hunks)webapp/src/component/searchSelect/InfiniteSearchSelect.tsx(6 hunks)webapp/src/constants/GlobalValidationSchema.tsx(1 hunks)webapp/src/constants/links.tsx(3 hunks)webapp/src/ee/branching/BranchesView.tsx(1 hunks)webapp/src/ee/branching/components/BranchForm.tsx(1 hunks)webapp/src/ee/branching/components/BranchItem.tsx(1 hunks)webapp/src/ee/branching/components/BranchModal.tsx(1 hunks)webapp/src/ee/branching/components/BranchNameChip.tsx(1 hunks)webapp/src/ee/branching/components/BranchesList.tsx(1 hunks)webapp/src/eeSetup/eeModule.ee.tsx(4 hunks)webapp/src/service/apiSchema.generated.ts(36 hunks)webapp/src/views/projects/ProjectRouter.tsx(1 hunks)webapp/src/views/projects/translations/KeyCreateForm/KeyCreateForm.tsx(3 hunks)webapp/src/views/projects/translations/TranslationHeader/TranslationControls.tsx(2 hunks)webapp/src/views/projects/translations/context/TranslationsContext.ts(3 hunks)webapp/src/views/projects/translations/context/services/useBranchesService.tsx(1 hunks)webapp/src/views/projects/translations/context/services/useEditService.tsx(3 hunks)webapp/src/views/projects/translations/context/services/useTranslationsService.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (28)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTranslationsTestData.kt (5)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/TestDataBuilder.kt (2)
addUserAccount(36-51)addProject(53-61)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (4)
addPermission(74-74)addEnglish(148-153)addGerman(155-161)addBranch(90-90)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt (1)
addLabel(131-134)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
addMeta(44-47)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (2)
addComment(56-64)addCodeReference(66-74)
webapp/src/views/projects/translations/TranslationHeader/TranslationControls.tsx (1)
webapp/src/component/branching/BranchSelector.tsx (1)
BranchSelector(12-39)
webapp/src/component/branching/BranchSelectField.tsx (1)
webapp/src/component/branching/BranchSelect.tsx (1)
BranchSelect(25-92)
webapp/src/ee/branching/BranchesView.tsx (3)
webapp/src/views/projects/BaseProjectView.tsx (1)
BaseProjectView(19-60)webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/ee/branching/components/BranchesList.tsx (1)
BranchesList(21-135)
webapp/src/views/projects/translations/context/services/useEditService.tsx (1)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt (1)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addBranch(90-90)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerModificationTest.kt (1)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (2)
saveTestData(77-83)saveTestData(85-121)
webapp/src/eeSetup/eeModule.ee.tsx (2)
webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/ee/branching/BranchesView.tsx (1)
BranchesView(7-27)
webapp/src/ee/branching/components/BranchModal.tsx (2)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6105)webapp/src/ee/branching/components/BranchForm.tsx (2)
BranchFormValues(13-16)BranchForm(18-63)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt (1)
backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addCodeReference(66-74)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
webapp/src/views/projects/translations/context/services/useTranslationsService.tsx (1)
useTranslationsService(74-435)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt (1)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addBranch(90-90)
webapp/src/ee/branching/components/BranchItem.tsx (4)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6105)webapp/src/ee/branching/components/BranchNameChip.tsx (1)
BranchNameChip(13-15)webapp/src/component/common/ClipboardCopy.tsx (1)
ClipboardCopy(13-53)webapp/src/component/branching/DefaultBranchChip.tsx (1)
DefaultBranchChip(5-8)
webapp/src/views/projects/translations/context/TranslationsContext.ts (2)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)webapp/src/views/projects/translations/context/services/useEditService.tsx (1)
useEditService(36-251)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (2)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addBranch(90-90)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
setDescription(100-103)
backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
filterBranch(267-284)
webapp/src/views/projects/ProjectRouter.tsx (2)
webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/views/projects/translations/TranslationsView.tsx (1)
TranslationsView(9-30)
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt (1)
filterBranch(158-174)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (6)
addBranch(90-90)addKey(82-82)addKey(105-111)addKey(113-115)addKey(117-128)addTranslation(130-130)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
setDescription(100-103)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addComment(56-64)
webapp/src/component/branching/BranchSelector.tsx (3)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/component/branching/BranchSelect.tsx (1)
BranchSelect(25-92)
webapp/src/views/projects/translations/context/services/useTranslationsService.tsx (1)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)
webapp/src/component/branching/BranchSelect.tsx (5)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)webapp/src/component/searchSelect/SearchSelect.tsx (1)
SelectItem(8-11)webapp/src/component/branching/DefaultBranchChip.tsx (1)
DefaultBranchChip(5-8)webapp/src/component/searchSelect/InfiniteSearchSelect.tsx (1)
InfiniteSearchSelect(58-209)webapp/src/component/branching/BranchLabel.tsx (1)
BranchLabel(12-27)
webapp/src/ee/branching/components/BranchForm.tsx (5)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6105)webapp/src/component/common/form/StandardForm.tsx (1)
StandardForm(37-121)webapp/src/constants/GlobalValidationSchema.tsx (1)
Validation(44-536)webapp/src/component/FormField.tsx (1)
FieldLabel(20-30)webapp/src/component/branching/BranchSelectField.tsx (1)
BranchSelectField(4-15)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (2)
create(123-157)create(192-202)
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (2)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt (1)
create(39-54)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
Key(41-131)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/fixtures/ProjectApiKeyAuthRequestPerformer.kt (1)
performProjectAuthGet(47-49)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (2)
createBranch(10-10)getBranch(9-9)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (2)
createBranch(16-18)getBranch(13-15)
webapp/src/ee/branching/components/BranchesList.tsx (6)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6105)webapp/src/service/http/useQueryApi.ts (2)
useApiMutation(224-265)useApiQuery(119-141)webapp/src/ee/branching/components/BranchForm.tsx (1)
BranchFormValues(13-16)webapp/src/hooks/confirmation.tsx (1)
confirmation(5-7)webapp/src/ee/branching/components/BranchItem.tsx (1)
BranchItem(47-76)webapp/src/ee/branching/components/BranchModal.tsx (1)
BranchModal(12-37)
🪛 Biome (2.1.2)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
[error] 30-30: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build backend 🏗️
| @ProjectJWTAuthTestMethod | ||
| @Test | ||
| fun `creates key with default branch (branch parameter not provided)`() { | ||
| performProjectAuthPost("keys", CreateKeyDto(name = "super_key_to_main_branch", branch = "dev")) | ||
| .andIsCreated.andAssertThatJson { | ||
| node("id").isValidId | ||
| node("name").isEqualTo("super_key_to_main_branch") | ||
| node("branch").isEqualTo("dev") | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix misleading test name.
The test name states "creates key with default branch (branch parameter not provided)" but the test actually provides an explicit branch parameter (branch = "dev"). This is misleading and contradicts the test's actual behavior.
Apply this diff to correct the test name:
- fun `creates key with default branch (branch parameter not provided)`() {
- performProjectAuthPost("keys", CreateKeyDto(name = "super_key_to_main_branch", branch = "dev"))
+ fun `creates key with explicit branch`() {
+ performProjectAuthPost("keys", CreateKeyDto(name = "super_key_to_dev_branch", branch = "dev"))
.andIsCreated.andAssertThatJson {
node("id").isValidId
- node("name").isEqualTo("super_key_to_main_branch")
+ node("name").isEqualTo("super_key_to_dev_branch")
node("branch").isEqualTo("dev")
}
}🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerCreationTest.kt
around lines 86 to 95, the test name claims it verifies creation with the
default branch when branch is not provided, but the test call supplies branch =
"dev"; update the test name to reflect that an explicit branch is provided
(e.g., rename the function to indicate "creates key with provided branch (branch
parameter provided)") so the name matches the test behavior.
| @Test | ||
| @ProjectJWTAuthTestMethod | ||
| fun `it search in default branch only`() { | ||
| saveAndPrepare() | ||
| performProjectAuthGet("keys/search?search=this-is-branched-key&languageTag=de").andAssertThatJson { | ||
| node("_embedded").isAbsent() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix test name consistency and verify test data setup.
-
Test name inconsistency: The test is named "it search in default branch only" but should be "it searches in default branch only" to match the naming pattern of other tests in this file (e.g., "it searches", "it searches for prefix").
-
Verify test data: Ensure that
KeySearchTestDatacreates a key named "this-is-branched-key" in a non-default branch. Without seeing the test data setup, it's unclear if this branched key actually exists, which could lead to a false positive test.
Apply this diff to fix the test name:
- fun `it search in default branch only`() {
+ fun `it searches in default branch only`() {Run the following script to verify the test data includes a branched key:
🏁 Script executed:
#!/bin/bash
# Verify that KeySearchTestData creates a branched key named "this-is-branched-key"
ast-grep --pattern $'class KeySearchTestData {
$$$
}'
rg -A 20 "class KeySearchTestData" --type=ktLength of output: 26
#!/bin/bash
# Locate KeySearchTestData definition
find . -type f -name "*KeySearchTestData*.kt"
# Inspect the class declaration for test data setup
rg -n "class KeySearchTestData" -C5 .
# Search for the branched key name within the repository
rg -n "this-is-branched-key" -C5 .🏁 Script executed:
#!/bin/bash
# Locate the KeySearchTestData Kotlin file
find . -type f -name "*KeySearchTestData*.kt"
# Search for the branched key literal across all Kotlin files
rg -n "this-is-branched-key" -g '*.kt'Length of output: 490
🏁 Script executed:
#!/bin/bash
# Show class header to see project and branch setup
sed -n '1,50p' backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt
# Inspect addKeyWithTranslations implementation
rg -n "fun addKeyWithTranslations" -C10 backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt
# Search for branch creation within the file
rg -n "branch" -C5 backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.ktLength of output: 2500
Rename test function for naming consistency
Apply the diff:
- fun `it search in default branch only`() {
+ fun `it searches in default branch only`() {🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerKeySearchTest.kt
around lines 148 to 155, rename the test function from `it search in default
branch only` to `it searches in default branch only` to match project naming
conventions; update the function declaration accordingly (keep annotations and
body unchanged) and ensure any references or usages are adjusted to the new
name.
| private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) { | ||
| val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull() | ||
| tagService.saveAll(metas.flatMap { it.tags }) | ||
| keyCodeReferenceRepository.saveAll(metas.flatMap { it.codeReferences }) | ||
| keyMetaService.saveAll(metas) | ||
| } |
There was a problem hiding this comment.
Potential FK/cascade issue: saving code references before metas
You persist codeReferences before KeyMeta, which can break referential integrity or double-persist with cascades. Save metas first, then code references (or rely solely on cascade). Also, align with import path which uses keyMetaService.saveAllCodeReferences.
private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) {
val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull()
tagService.saveAll(metas.flatMap { it.tags })
- keyCodeReferenceRepository.saveAll(metas.flatMap { it.codeReferences })
- keyMetaService.saveAll(metas)
+ // persist metas first to ensure FK targets exist
+ keyMetaService.saveAll(metas)
+ // unify with import path saving approach
+ keyMetaService.saveAllCodeReferences(metas.flatMap { it.codeReferences })
}If you adopt this, the KeyCodeReferenceRepository dependency becomes unused; remove it to avoid dead deps.
📝 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.
| private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) { | |
| val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull() | |
| tagService.saveAll(metas.flatMap { it.tags }) | |
| keyCodeReferenceRepository.saveAll(metas.flatMap { it.codeReferences }) | |
| keyMetaService.saveAll(metas) | |
| } | |
| private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) { | |
| val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull() | |
| tagService.saveAll(metas.flatMap { it.tags }) | |
| // persist metas first to ensure FK targets exist | |
| keyMetaService.saveAll(metas) | |
| // unify with import path saving approach | |
| keyMetaService.saveAllCodeReferences(metas.flatMap { it.codeReferences }) | |
| } |
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt
around lines 465 to 470, the current save order persists codeReferences before
KeyMeta which can break FK integrity or cause double-persist with cascades;
change the order to persist metas first via keyMetaService.saveAll(metas) and
then save code references using
keyMetaService.saveAllCodeReferences(metas.flatMap { it.codeReferences }) (or
rely solely on cascade and omit explicit codeReferences save), and after
switching to keyMetaService for code references remove the now-unused
keyCodeReferenceRepository dependency/import to avoid dead code.
| @ActivityLoggedProp | ||
| @Column(length = 200) | ||
| var name: String = "", | ||
|
|
||
| @ActivityLoggedProp | ||
| var isDefault: Boolean = false, | ||
|
|
||
| @ActivityLoggedProp | ||
| var isProtected: Boolean = false, | ||
|
|
||
| @ActivityLoggedProp | ||
| var archivedAt: Date? = null, | ||
|
|
||
| @Column(name = "pending") | ||
| var pending: Boolean = false, | ||
|
|
There was a problem hiding this comment.
Nullability mismatch for name and pending can cause NPEs
- Kotlin fields are non-null (name: String, pending: Boolean), but DB columns are nullable in schema.xml. Hydration can produce nulls and NPEs.
Code-side:
- @Column(length = 200)
+ @Column(length = 200, nullable = false)
var name: String = "",
@@
- @Column(name = "pending")
- var pending: Boolean = false,
+ @Column(name = "pending", nullable = false)
+ var pending: Boolean = false,Schema-side (see schema.xml comments below) ensure:
- branch.name NOT NULL
- branch.pending NOT NULL DEFAULT false
| @OneToMany(fetch = FetchType.LAZY, cascade = [CascadeType.PERSIST], mappedBy = "project") | ||
| @SQLRestriction("archived_at IS NULL") | ||
| var branches: MutableList<Branch> = mutableListOf() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the SQLRestriction column name.
The @SQLRestriction annotation uses "archived_at IS NULL". Ensure this column name matches the actual database column name in the Branch entity. Hibernate typically uses snake_case for column names by default, but it's worth confirming that the Branch.archivedAt field maps to archived_at in the database schema.
Run the following script to verify the column mapping:
🏁 Script executed:
#!/bin/bash
# Description: Check Branch entity's archivedAt column mapping
# Search for the archivedAt field definition in Branch entity
ast-grep --pattern $'class Branch {
$$$
archivedAt$$$
$$$
}'
# Also search for any @Column annotations on archivedAt
rg -n '@Column.*archivedAt|archivedAt.*@Column' --type kotlin -A 2 -B 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate the Kotlin file defining Branch
branch_file=$(rg -l "class Branch" -g "*.kt")
echo "Branch file: $branch_file"
# Search for archivedAt definition and annotations
rg -n "archivedAt" -C3 "$branch_file"Length of output: 2384
SQLRestriction references nonexistent column archived_at in Branch
The filter @SQLRestriction("archived_at IS NULL") on branches points to a column not mapped in Branch.kt (no archivedAt property or @Column(name = "archived_at")). Add the field/mapping or remove/update the restriction.
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/Project.kt around lines 148-150,
the @SQLRestriction("archived_at IS NULL") on the branches collection refers to
a non-existent column in Branch; fix by either adding a nullable mapped property
in Branch (e.g., an Instant/LocalDateTime or appropriate date-time type with
@Column(name = "archived_at") and nullable) so the column exists for the
restriction, or remove/update the @SQLRestriction to reference an existing
mapped column; ensure the chosen fix is applied consistently (add the field to
Branch.kt with proper import/annotations and database migration, or
delete/change the restriction in Project.kt).
| const match = useRouteMatch(); | ||
| const routeBranch = match.params[PARAMS.TRANSLATIONS_BRANCH]; | ||
|
|
||
| projectId = projectId || useProject().id; |
There was a problem hiding this comment.
Hooks rule violation: calling a hook conditionally
useProject() is invoked conditionally via projectId || useProject().id, breaking hooks order. Call it unconditionally and derive an id.
Apply:
-import { useProject } from 'tg.hooks/useProject';
+import { useProject } from 'tg.hooks/useProject';
@@
-export const useBranchesService = ({ projectId, enabled = true }: Props) => {
+export const useBranchesService = ({ projectId, enabled = true }: Props) => {
const match = useRouteMatch();
const routeBranch = match.params[PARAMS.TRANSLATIONS_BRANCH];
-
- projectId = projectId || useProject().id;
+ const projectCtx = useProject();
+ const resolvedProjectId = projectId ?? projectCtx.id;
@@
- path: { projectId: projectId },
+ path: { projectId: resolvedProjectId },🧰 Tools
🪛 Biome (2.1.2)
[error] 30-30: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
around line 30, the code conditionally calls the hook useProject() via projectId
|| useProject().id which violates React hooks rules; always call useProject()
unconditionally and then derive the id by using const project = useProject();
projectId = projectId || project.id (or better, compute a local const
resolvedProjectId = projectId ?? project.id and use that) so hooks order stays
stable.
| const loadableBranches = useApiInfiniteQuery({ | ||
| url: '/v2/projects/{projectId}/branches', | ||
| method: 'get', | ||
| path: { projectId: projectId }, | ||
| query: { | ||
| size: 10000, | ||
| }, | ||
| options: { | ||
| keepPreviousData: true, | ||
| refetchOnMount: true, | ||
| noGlobalLoading: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Pass through the enabled flag to the query
You accept enabled but don’t use it. Wire it to query options.
const loadableBranches = useApiInfiniteQuery({
@@
- options: {
+ options: {
keepPreviousData: true,
refetchOnMount: true,
noGlobalLoading: true,
+ enabled,
},
});📝 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.
| const loadableBranches = useApiInfiniteQuery({ | |
| url: '/v2/projects/{projectId}/branches', | |
| method: 'get', | |
| path: { projectId: projectId }, | |
| query: { | |
| size: 10000, | |
| }, | |
| options: { | |
| keepPreviousData: true, | |
| refetchOnMount: true, | |
| noGlobalLoading: true, | |
| }, | |
| }); | |
| const loadableBranches = useApiInfiniteQuery({ | |
| url: '/v2/projects/{projectId}/branches', | |
| method: 'get', | |
| path: { projectId: projectId }, | |
| query: { | |
| size: 10000, | |
| }, | |
| options: { | |
| keepPreviousData: true, | |
| refetchOnMount: true, | |
| noGlobalLoading: true, | |
| enabled, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
around lines 32 to 44, the hook accepts an enabled parameter but doesn't pass it
to the query options; update the useApiInfiniteQuery call to forward the enabled
flag by adding options.enabled: enabled (or options.enabled: Boolean(enabled) if
needed) so the query respects the caller's enabled state while preserving the
existing options (keepPreviousData, refetchOnMount, noGlobalLoading).
| const loadedBranches = loadableBranches.data?.pages.flatMap( | ||
| (p) => p._embedded?.branches ?? [] | ||
| ); | ||
|
|
||
| const data = | ||
| loadableBranches.isFetched && loadedBranches!.length > 0 | ||
| ? loadableBranches.data!.pages.flatMap((p) => p._embedded?.branches ?? []) | ||
| : Array.from([defaultBranchObject]); | ||
|
|
There was a problem hiding this comment.
Avoid unsafe non-null assertion and crash when request errors
isFetched && loadedBranches!.length > 0 can throw when data is undefined (errors still set isFetched=true). Use optional chaining and/or isSuccess.
- const loadedBranches = loadableBranches.data?.pages.flatMap(
- (p) => p._embedded?.branches ?? []
- );
+ const loadedBranches =
+ loadableBranches.data?.pages.flatMap((p) => p._embedded?.branches ?? []) ??
+ [];
@@
- const data =
- loadableBranches.isFetched && loadedBranches!.length > 0
- ? loadableBranches.data!.pages.flatMap((p) => p._embedded?.branches ?? [])
- : Array.from([defaultBranchObject]);
+ const data =
+ (loadableBranches.isSuccess && loadedBranches.length > 0 && loadedBranches) ||
+ [defaultBranchObject];📝 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.
| const loadedBranches = loadableBranches.data?.pages.flatMap( | |
| (p) => p._embedded?.branches ?? [] | |
| ); | |
| const data = | |
| loadableBranches.isFetched && loadedBranches!.length > 0 | |
| ? loadableBranches.data!.pages.flatMap((p) => p._embedded?.branches ?? []) | |
| : Array.from([defaultBranchObject]); | |
| const loadedBranches = | |
| loadableBranches.data?.pages.flatMap((p) => p._embedded?.branches ?? []) ?? []; | |
| const data = | |
| (loadableBranches.isSuccess && loadedBranches.length > 0 && loadedBranches) || | |
| [defaultBranchObject]; |
🤖 Prompt for AI Agents
In
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
around lines 46 to 54, the condition uses a non-null assertion (loadedBranches!)
and can throw when data is undefined even though isFetched is true; replace that
check with a safe one (e.g. loadableBranches.isSuccess &&
(loadedBranches?.length ?? 0) > 0) or explicitly check loadedBranches?.length >
0, and reuse the already computed loadedBranches variable for the true branch
instead of recomputing flatMap; ensure you never use "!" on loadedBranches and
always guard access with optional chaining or isSuccess so the fallback
defaultBranchObject is used when data is missing or the request errored.
| const urlBranch = routeBranch | ||
| ? data?.find((b) => b.name === routeBranch) | ||
| : undefined; | ||
|
|
||
| const defaultBranch = loadableBranches.isFetched | ||
| ? loadedBranches?.find((b) => b.isDefault) || defaultBranchObject | ||
| : null; | ||
|
|
||
| const selected = routeBranch ? (urlBranch ? urlBranch : null) : defaultBranch; | ||
|
|
||
| return { | ||
| branches: data, | ||
| selected: selected, | ||
| default: defaultBranch, | ||
| selectedName: routeBranch, | ||
| loadable: loadableBranches, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
selectedName can become invalid; return the actual selected name
If URL branch is unknown, selected becomes null but selectedName stays as the invalid URL value, causing queries to filter by a non-existent branch and return no data.
Also, prefer useParams over useRouteMatch for params access.
-import { useRouteMatch } from 'react-router-dom';
+import { useParams } from 'react-router-dom';
@@
- const match = useRouteMatch();
- const routeBranch = match.params[PARAMS.TRANSLATIONS_BRANCH];
+ const params = useParams<Record<string, string | undefined>>();
+ const routeBranch = params[PARAMS.TRANSLATIONS_BRANCH];
@@
- const selected = routeBranch ? (urlBranch ? urlBranch : null) : defaultBranch;
+ const selected = routeBranch ? (urlBranch ?? null) : defaultBranch;
@@
- selectedName: routeBranch,
+ selectedName: selected?.name,To ensure no callers rely on the previous selectedName behavior or on optional projectId, list usages:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=tsx -C2 '\buseBranchesService\s*\('Length of output: 27
🏁 Script executed:
#!/bin/bash
rg -n '\buseBranchesService\s*\(' -C2Length of output: 1795
🏁 Script executed:
#!/bin/bash
echo "---- useBranchesService.tsx (lines 1-80) ----"
sed -n '1,80p' webapp/src/views/projects/translations/context/services/useBranchesService.tsx
echo "---- useTranslationsService.tsx (lines 70-110) ----"
sed -n '70,110p' webapp/src/views/projects/translations/context/services/useTranslationsService.tsxLength of output: 3357
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' webapp/src/views/projects/translations/context/services/useTranslationsService.tsxLength of output: 6125
Update useBranchesService to return the actual branch name and use useParams
- Replace useRouteMatch with useParams to read URL params
- Derive selectedName from selected?.name so it becomes undefined when the branch is invalid
- (Optional) simplify the selected expression using nullish coalescing
-import { useRouteMatch } from 'react-router-dom';
+import { useParams } from 'react-router-dom';
@@
- const match = useRouteMatch();
- const routeBranch = match.params[PARAMS.TRANSLATIONS_BRANCH];
+ const params = useParams<Record<string, string | undefined>>();
+ const routeBranch = params[PARAMS.TRANSLATIONS_BRANCH];
@@
- const selected = routeBranch ? (urlBranch ? urlBranch : null) : defaultBranch;
+ const selected = routeBranch ? (urlBranch ?? null) : defaultBranch;
@@
- selectedName: routeBranch,
+ selectedName: selected?.name,📝 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.
| const urlBranch = routeBranch | |
| ? data?.find((b) => b.name === routeBranch) | |
| : undefined; | |
| const defaultBranch = loadableBranches.isFetched | |
| ? loadedBranches?.find((b) => b.isDefault) || defaultBranchObject | |
| : null; | |
| const selected = routeBranch ? (urlBranch ? urlBranch : null) : defaultBranch; | |
| return { | |
| branches: data, | |
| selected: selected, | |
| default: defaultBranch, | |
| selectedName: routeBranch, | |
| loadable: loadableBranches, | |
| }; | |
| // At the top of the file | |
| import { useParams } from 'react-router-dom'; | |
| @@ | |
| - const match = useRouteMatch(); | |
| const params = useParams<Record<string, string | undefined>>(); | |
| const routeBranch = params[PARAMS.TRANSLATIONS_BRANCH]; | |
| const urlBranch = routeBranch | |
| ? data?.find((b) => b.name === routeBranch) | |
| : undefined; | |
| const defaultBranch = loadableBranches.isFetched | |
| ? loadedBranches?.find((b) => b.isDefault) || defaultBranchObject | |
| : null; | |
| const selected = routeBranch ? (urlBranch ?? null) : defaultBranch; | |
| return { | |
| branches: data, | |
| selected: selected, | |
| default: defaultBranch, | |
| selectedName: selected?.name, | |
| loadable: loadableBranches, | |
| }; |
🤖 Prompt for AI Agents
In
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
around lines 55 to 71, replace useRouteMatch usage with useParams to read the
branch param from the URL, derive selectedName from selected?.name (so it
becomes undefined when the branch is invalid) instead of returning the raw route
param, and simplify selected using nullish coalescing (e.g. selected = urlBranch
?? defaultBranch) while updating imports to import useParams from react-router
(and removing useRouteMatch). Ensure the returned selectedName is the actual
branch name or undefined when invalid and adjust any types accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (1)
89-105: Race condition in branch existence check remains unaddressed.The direct assertion at line 104 can race with asynchronous deletion. Wrap the branch existence check in a wait loop as previously suggested.
🧹 Nitpick comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (1)
122-123: Variable names are backwards in assertIsCopyOf.The variable naming is confusing:
mainTranslationsis assigned fromthis.translations(the branch/copy key), whilebranchTranslationsis assigned fromother.translations(the main/original key). This naming is backwards and appears in multiple places (translations, screenshots, code references).Consider renaming for clarity:
- val mainTranslations = this.translations - val branchTranslations = other.translations + val copyTranslations = this.translations + val originalTranslations = other.translationsApply similar changes to screenshot and code reference variables at lines 167-168 and 176-177.
Also applies to: 167-168, 176-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (4)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (4)
22-44: LGTM! Test infrastructure properly configured.The SpringBootTest configuration with TRACE logging, repository autowiring, and test data setup are all correctly implemented.
46-72: LGTM! Comprehensive validation of branch copy functionality.The test properly validates branch creation, key count, key copying, and branch readiness state.
74-87: LGTM! Performance validation is appropriate.The performance test with 2000 keys and a 3-second threshold provides good regression protection for the copy operation.
107-119: LGTM! Helper methods are clean and focused.The helper methods appropriately encapsulate HTTP interactions for branch creation and deletion.
| val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id)) | ||
| val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id)) | ||
| mainKeyScreenshots.first().let { screenshotReference -> | ||
| val branchScreenshotReference = branchKeyScreenshots.first() | ||
| screenshotReference.screenshot.assert.isEqualTo(branchScreenshotReference.screenshot) | ||
| screenshotReference.originalText.assert.isEqualTo(branchScreenshotReference.originalText) | ||
| screenshotReference.positions.assert.isEqualTo(branchScreenshotReference.positions) | ||
| } |
There was a problem hiding this comment.
Add size check before accessing first screenshot.
Line 169 calls .first() on mainKeyScreenshots without verifying the collection is non-empty, which could cause NoSuchElementException. This is inconsistent with the code references section (line 179) which includes a size check.
Apply this diff:
val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id))
val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id))
+ mainKeyScreenshots.assert.hasSizeGreaterThan(0)
+ mainKeyScreenshots.assert.hasSize(branchKeyScreenshots.size)
mainKeyScreenshots.first().let { screenshotReference ->📝 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.
| val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id)) | |
| val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id)) | |
| mainKeyScreenshots.first().let { screenshotReference -> | |
| val branchScreenshotReference = branchKeyScreenshots.first() | |
| screenshotReference.screenshot.assert.isEqualTo(branchScreenshotReference.screenshot) | |
| screenshotReference.originalText.assert.isEqualTo(branchScreenshotReference.originalText) | |
| screenshotReference.positions.assert.isEqualTo(branchScreenshotReference.positions) | |
| } | |
| val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id)) | |
| val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id)) | |
| mainKeyScreenshots.assert.hasSizeGreaterThan(0) | |
| mainKeyScreenshots.assert.hasSize(branchKeyScreenshots.size) | |
| mainKeyScreenshots.first().let { screenshotReference -> | |
| val branchScreenshotReference = branchKeyScreenshots.first() | |
| screenshotReference.screenshot.assert.isEqualTo(branchScreenshotReference.screenshot) | |
| screenshotReference.originalText.assert.isEqualTo(branchScreenshotReference.originalText) | |
| screenshotReference.positions.assert.isEqualTo(branchScreenshotReference.positions) | |
| } |
🤖 Prompt for AI Agents
In
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt
around lines 167 to 174, the test calls .first() on mainKeyScreenshots and
branchKeyScreenshots without verifying they contain elements; add an explicit
size check or assertion before accessing .first() (matching the style used
around line 179) — for example assertThat(mainKeyScreenshots).hasSize(1) (or
assertTrue/isNotEmpty) and similarly for branchKeyScreenshots, then safely call
.first() (or use firstOrNull with assertNotNull) and keep the subsequent field
equality assertions.
0c36436 to
28f666d
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/data/src/main/kotlin/io/tolgee/dtos/IExportParams.kt (1)
109-125: Add missingfilterBranchassignment incopyPropsFrom
InIExportParams.kt, includethis.filterBranch = other.filterBranchinside
copyPropsFromto preserve branch filtering.
♻️ Duplicate comments (20)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
8-23: The critical bean collision issue has already been identified.The previous review comment comprehensively covers the missing
@ConditionalOnMissingBeanannotation that prevents bean conflicts in EE builds, as well as the improvement of exception messages. Please refer to the existing comment for the complete fix.webapp/src/eeSetup/eeModule.ee.tsx (1)
404-412: Protect the branches menu behind real permissionsLeaving
condition: ({ satisfiesPermission }) => trueships the branches menu (and its CRUD view) to every project member, bypassing authorization entirely. Wire the actual permission (and optional feature flag) before merging.backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerKeySearchTest.kt (1)
150-155: Fix grammar of the test name.Please rename the display name to
it searches in default branch onlyto stay consistent with the naming pattern used across this suite and to resolve the earlier feedback.webapp/src/component/branching/BranchSelector.tsx (1)
17-23: Move side effect to useEffect to avoid render-phase mutations.The redirect logic executes during render, which can cause React warnings about state updates during render and potential infinite loops.
As noted in the previous review, wrap this in useEffect:
+ React.useEffect(() => { if (!loadable.isLoading && !selected) { history.replace( LINKS.PROJECT_TRANSLATIONS.build({ [PARAMS.PROJECT_ID]: project.id, }) ); } + }, [loadable.isLoading, selected, history, project.id]);backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (1)
465-470: Fix save order to prevent FK violations.Persisting code references before their parent KeyMeta entities can break referential integrity constraints. The KeyMeta must be saved first to establish the FK target before saving code references.
As noted in the previous review, change the save order:
private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) { val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull() tagService.saveAll(metas.flatMap { it.tags }) - keyCodeReferenceRepository.saveAll(metas.flatMap { it.codeReferences }) keyMetaService.saveAll(metas) + keyMetaService.saveAllCodeReferences(metas.flatMap { it.codeReferences }) }This aligns with the import path at line 476 which uses
keyMetaService.saveAllCodeReferences. After this change,keyCodeReferenceRepositorydependency becomes unused and should be removed.backend/data/src/main/kotlin/io/tolgee/service/bigMeta/KeysDistanceUtil.kt (1)
46-52: Use item2.branch for key2 lookup (not item1.branch).This mixes branches when resolving
key2Id, producing wrong distances across branches.- val key2Id = - getKeyId(item2.namespace, item2.keyName, item1.branch ?: project.getDefaultBranch()?.name) ?: return@forEach2 + val key2Id = + getKeyId(item2.namespace, item2.keyName, item2.branch ?: project.getDefaultBranch()?.name) ?: return@forEach2backend/data/src/main/kotlin/io/tolgee/model/Project.kt (1)
148-150: This issue was previously flagged in a past review comment. The@SQLRestriction("archived_at IS NULL")references a column that must be mapped in theBranchentity. Based on test data (e.g.,BranchTestData.ktline 39, 127), thearchivedAtproperty appears to exist inBranch, but the past review requested verification that the column mapping is correct.ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/BranchRepository.kt (1)
12-20: This issue was flagged in a past review. Thesearchparameter is unused in the query, andPageable?should likely be non-nullable. The past review suggested adding a search filter to the query and changing the signature to accept non-nullPageable.webapp/src/component/branching/BranchSelect.tsx (1)
45-50: This issue was flagged in a past review. TheuseEffectdepends only ondefaultBranch, but it should depend ondefaultValue(andonDefaultValue) to properly sync when thebranchprop changes. The past review suggested updating the dependency array.webapp/src/views/projects/translations/context/services/useBranchesService.tsx (4)
26-30: This issue was flagged in a past review and by static analysis. The conditional hook callprojectId || useProject().idviolates React hooks rules. The past review suggested callinguseProject()unconditionally and then deriving the resolved ID.
32-44: This issue was flagged in a past review. Theenabledprop is accepted but not passed to the query options, so the query can't be conditionally disabled. The past review suggested addingenabledto the options object.
46-54: This issue was flagged in a past review. The code uses a non-null assertion (loadedBranches!) which can throw when the query errors (sinceisFetchedcan be true even whendatais undefined). The past review suggested usingisSuccessand optional chaining.
55-71: This issue was flagged in a past review. TheselectedNamereturns the raw URL parameter even when the branch doesn't exist (selected becomes null). The past review suggested returningselected?.nameinstead and usinguseParamsrather thanuseRouteMatch.backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt (1)
288-305: Risk of MultipleBagFetchExceptionThis query fetch-joins multiple collections (
translations,t.labels,t.comments,km.tags) which can cause Hibernate's MultipleBagFetchException or cartesian product issues.ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (2)
102-109: Wrap branch existence check in wait loopDeletion is event-driven and may race. The branch existence check on line 108 should be wrapped in
waitForNotThrowingsimilar to the key count check to avoid test flakiness.
171-178: Add size check before accessing first screenshot.Calling
.first()onmainKeyScreenshotsandbranchKeyScreenshotswithout verifying the collections are non-empty can throwNoSuchElementException.backend/data/src/main/kotlin/io/tolgee/model/branching/Branch.kt (1)
41-56: Nullability mismatch for name and pending can cause NPEsKotlin fields
name: Stringandpending: Booleanare non-null, but@Columnannotations lacknullable = false. This mismatch can cause NPEs during entity hydration if DB columns allow NULL.backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (1)
242-247: Early return ignores branch contextThe early return only checks
nameandnamespaceequality, ignoring thebranchparameter. If branch changes while name/namespace remain the same, necessary work (uniqueness check, branch assignment) is skipped.backend/data/src/main/resources/db/changelog/schema.xml (1)
4658-4662: Still missing NOT NULL/default on branch.name & pendingBranch.name and Branch.pending are non-nullable in Kotlin (pending defaults to false), but the DDL still leaves both columns nullable and pending without a default. That leaks NULLs into an object graph the rest of the code treats as non-null, risking runtime NPEs and violating business rules. Please enforce the constraints and default directly in the changelog.
- <column name="name" type="VARCHAR(200)"/> + <column name="name" type="VARCHAR(200)"> + <constraints nullable="false"/> + </column> ... - <addColumn tableName="branch"> - <column name="pending" type="BOOLEAN"/> - </addColumn> + <addColumn tableName="branch"> + <column name="pending" type="BOOLEAN" defaultValueBoolean="false"> + <constraints nullable="false"/> + </column> + </addColumn>Because the table is new in this release, no backfill changeSet is required after setting the default.
Also applies to: 4701-4703
webapp/src/service/apiSchema.generated.ts (1)
2493-2497: Map new branch-related error codes in UIAdd UX mappings for: unsupported_media_type, branch_not_found, cannot_delete_default_branch, branch_already_exists, origin_branch_not_found.
#!/bin/bash # Look for error-code mapping usage rg -nP --type=ts -C3 'branch_not_found|cannot_delete_default_branch|branch_already_exists|origin_branch_not_found|unsupported_media_type'Also applies to: 5592-5596
🧹 Nitpick comments (4)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt (1)
354-354: Remove redundant author assignments.The
author = userAccountassignments inside theaddCodeReferencelambda blocks are redundant becauseuserAccountis already passed as the first parameter toaddCodeReference(userAccount), which sets the author via the constructor. Setting it again in the lambda body is unnecessary and violates DRY.Apply this diff to remove the redundant assignments:
addCodeReference(userAccount) { path = "./code/exist.extension" line = 10 fromImport = true - author = userAccount }addCodeReference(userAccount) { path = "./code/exist.extension" line = 10 fromImport = true - author = userAccount }addCodeReference(userAccount) { path = "./code/notExist.extendison" fromImport = false - author = userAccount }addCodeReference(userAccount) { path = "./code/exist.extension" line = 10 fromImport = true - author = userAccount }Also applies to: 375-375, 380-380, 393-393
webapp/src/component/branching/BranchLabel.tsx (1)
21-21: Avoidanytype assertion.The type assertion
(rest as any)bypasses TypeScript's type checking. Define a proper type for the component props or use a more specific type.Apply this diff:
-export const BranchLabel = React.forwardRef(function BranchLabel( - { value, placeholder, ...rest }: InputBaseComponentProps, - ref -) { +export const BranchLabel = React.forwardRef< + HTMLDivElement, + InputBaseComponentProps +>(function BranchLabel({ value, placeholder, ...rest }, ref) { return ( <StyledFakeInput tabIndex={0} ref={ref} style={{ display: 'flex' }} - {...(rest as any)} + {...rest} > <GitBranch02 height={20} width={20} style={{ marginRight: 10 }} /> <div>{value || placeholder}</div> </StyledFakeInput> ); });backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt (2)
88-91: Disambiguate receiver: usethis@build.selffor clarity.Current
apply { branch = self }relies on implicit receiver resolution; make it explicit to avoid confusion in DSLs with multiple receivers.- addBasicKey().apply { - branch = self - } + addBasicKey().apply { + branch = this@build.self + }
135-136: Redundant manual list mutation; likely duplicates the translation in memory.
addTranslation { ... }should already manage the relationship. Manually addingself.translations.add(this)risks duplicates in the list.- self.translations.add(this)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (104)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/keys/KeyController.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/TranslationsController.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/component/KeyComplexEditHelper.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/controllers/ExportController.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/contentDelivery/ContentDeliveryConfigModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModelAssembler.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithDataModel.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithDataModelAssembler.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithScreenshotsModelAssembler.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/AllKeysControllerTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerModificationTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ImportController/SingleStepImportControllerTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerCreationTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerKeySearchTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerTest.kt(4 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ProjectsController/ProjectsControllerCreateTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/service/LanguageServiceTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/service/TranslationServiceTest.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/constants/Message.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt(5 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchBuilder.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTranslationsTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/ExportParamsDocs.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/IExportParams.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/RelatedKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/KeyIdFindResult.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/KeyView.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/ContentDeliveryConfigRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/export/ExportParams.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/ComplexEditKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/CreateKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/key/EditKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/SetTranslationsWithKeyDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/TranslationFilters.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/importKeysResolvable/ImportKeysResolvableItemDto.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/events/OnBranchDeleted.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/Project.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/Branch.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/repository/KeyMetaRepository.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt(9 hunks)backend/data/src/main/kotlin/io/tolgee/repository/TranslationRepository.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/bigMeta/BigMetaService.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/bigMeta/KeysDistanceUtil.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchCopyService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt(5 hunks)backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt(6 hunks)backend/data/src/main/kotlin/io/tolgee/service/key/ResolvingKeyImporter.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/project/ProjectCreationService.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/LanguageStatsProvider.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/ProjectStatsProvider.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/translation/TranslationService.kt(2 hunks)backend/data/src/main/resources/db/changelog/schema.xml(1 hunks)backend/data/src/test/kotlin/io/tolgee/unit/bigMeta/KeysDistanceUnitTestData.kt(1 hunks)e2e/cypress/support/dataCyType.d.ts(2 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/BranchModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/CreateBranchModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/BranchRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCleanupService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/DefaultBranchCreator.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)webapp/src/component/branching/BranchLabel.tsx(1 hunks)webapp/src/component/branching/BranchSelect.tsx(1 hunks)webapp/src/component/branching/BranchSelectField.tsx(1 hunks)webapp/src/component/branching/BranchSelector.tsx(1 hunks)webapp/src/component/branching/DefaultBranchChip.tsx(1 hunks)webapp/src/component/common/ClipboardCopy.tsx(1 hunks)webapp/src/component/searchSelect/InfiniteSearchSelect.tsx(6 hunks)webapp/src/constants/GlobalValidationSchema.tsx(1 hunks)webapp/src/constants/links.tsx(3 hunks)webapp/src/ee/branching/BranchesView.tsx(1 hunks)webapp/src/ee/branching/components/BranchForm.tsx(1 hunks)webapp/src/ee/branching/components/BranchItem.tsx(1 hunks)webapp/src/ee/branching/components/BranchModal.tsx(1 hunks)webapp/src/ee/branching/components/BranchNameChip.tsx(1 hunks)webapp/src/ee/branching/components/BranchesList.tsx(1 hunks)webapp/src/eeSetup/eeModule.ee.tsx(4 hunks)webapp/src/service/apiSchema.generated.ts(36 hunks)webapp/src/views/projects/ProjectRouter.tsx(1 hunks)webapp/src/views/projects/translations/KeyCreateForm/KeyCreateForm.tsx(3 hunks)webapp/src/views/projects/translations/TranslationHeader/TranslationControls.tsx(2 hunks)webapp/src/views/projects/translations/context/TranslationsContext.ts(3 hunks)webapp/src/views/projects/translations/context/services/useBranchesService.tsx(1 hunks)webapp/src/views/projects/translations/context/services/useEditService.tsx(3 hunks)webapp/src/views/projects/translations/context/services/useTranslationsService.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- webapp/src/ee/branching/components/BranchItem.tsx
🚧 Files skipped from review as they are similar to previous changes (50)
- backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/SetTranslationsWithKeyDto.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/ExportParamsDocs.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ImportController/SingleStepImportControllerTest.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/RelatedKeyDto.kt
- backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/KeyView.kt
- webapp/src/views/projects/translations/context/services/useEditService.tsx
- backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModelAssembler.kt
- backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithDataModel.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/request/export/ExportParams.kt
- webapp/src/views/projects/translations/KeyCreateForm/KeyCreateForm.tsx
- backend/app/src/test/kotlin/io/tolgee/service/LanguageServiceTest.kt
- webapp/src/views/projects/translations/context/TranslationsContext.ts
- backend/data/src/main/kotlin/io/tolgee/service/project/ProjectCreationService.kt
- webapp/src/views/projects/ProjectRouter.tsx
- backend/data/src/main/kotlin/io/tolgee/dtos/request/key/EditKeyDto.kt
- backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyWithScreenshotsModelAssembler.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/TranslationFilters.kt
- webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
- e2e/cypress/support/dataCyType.d.ts
- webapp/src/ee/branching/components/BranchNameChip.tsx
- backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt
- backend/data/src/main/kotlin/io/tolgee/constants/Message.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/request/translation/importKeysResolvable/ImportKeysResolvableItemDto.kt
- backend/data/src/main/kotlin/io/tolgee/repository/KeyMetaRepository.kt
- backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeySearchTestData.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/AllKeysControllerTest.kt
- backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt
- webapp/src/constants/GlobalValidationSchema.tsx
- backend/data/src/main/kotlin/io/tolgee/dtos/request/key/CreateKeyDto.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerCreationTest.kt
- backend/api/src/main/kotlin/io/tolgee/hateoas/key/KeyModel.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/BranchModelAssembler.kt
- backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTranslationsTestData.kt
- webapp/src/views/projects/translations/TranslationHeader/TranslationControls.tsx
- backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/request/key/ComplexEditKeyDto.kt
- webapp/src/ee/branching/components/BranchesList.tsx
- backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/LanguageStatsProvider.kt
- webapp/src/component/branching/DefaultBranchChip.tsx
- backend/data/src/main/kotlin/io/tolgee/service/key/ResolvingKeyImporter.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCleanupService.kt
- webapp/src/ee/branching/components/BranchModal.tsx
- backend/api/src/main/kotlin/io/tolgee/hateoas/contentDelivery/ContentDeliveryConfigModel.kt
- backend/data/src/main/kotlin/io/tolgee/dtos/request/ContentDeliveryConfigRequest.kt
- webapp/src/component/common/ClipboardCopy.tsx
- backend/app/src/test/kotlin/io/tolgee/service/TranslationServiceTest.kt
🧰 Additional context used
🧬 Code graph analysis (16)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/dataImport/ImportTestData.kt (1)
backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addCodeReference(66-74)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (6)
addBranch(90-90)addKey(82-82)addKey(105-111)addKey(113-115)addKey(117-128)addTranslation(130-130)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
setDescription(100-103)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addComment(56-64)
webapp/src/ee/branching/BranchesView.tsx (3)
webapp/src/views/projects/BaseProjectView.tsx (1)
BaseProjectView(19-60)webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/ee/branching/components/BranchesList.tsx (1)
BranchesList(21-135)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (2)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addBranch(90-90)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
setDescription(100-103)
webapp/src/component/branching/BranchSelect.tsx (5)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)webapp/src/component/searchSelect/SearchSelect.tsx (1)
SelectItem(8-11)webapp/src/component/branching/DefaultBranchChip.tsx (1)
DefaultBranchChip(5-8)webapp/src/component/searchSelect/InfiniteSearchSelect.tsx (1)
InfiniteSearchSelect(58-209)webapp/src/component/branching/BranchLabel.tsx (1)
BranchLabel(12-27)
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (2)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt (1)
create(39-54)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
Key(41-131)
backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
filterBranch(267-284)
webapp/src/component/branching/BranchSelector.tsx (4)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6105)webapp/src/views/projects/translations/context/services/useBranchesService.tsx (1)
useBranchesService(26-72)webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/component/branching/BranchSelect.tsx (1)
BranchSelect(25-92)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt (2)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (5)
addBranch(90-90)addKey(82-82)addKey(105-111)addKey(113-115)addKey(117-128)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/TestDataBuilder.kt (1)
addProject(53-61)
webapp/src/component/branching/BranchSelectField.tsx (1)
webapp/src/component/branching/BranchSelect.tsx (1)
BranchSelect(25-92)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (2)
create(123-157)create(192-202)
webapp/src/ee/branching/components/BranchForm.tsx (3)
webapp/src/component/common/form/StandardForm.tsx (1)
StandardForm(37-121)webapp/src/constants/GlobalValidationSchema.tsx (1)
Validation(44-536)webapp/src/component/branching/BranchSelectField.tsx (1)
BranchSelectField(4-15)
webapp/src/eeSetup/eeModule.ee.tsx (2)
webapp/src/constants/links.tsx (1)
LINKS(69-441)webapp/src/ee/branching/BranchesView.tsx (1)
BranchesView(7-27)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerModificationTest.kt (1)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (2)
saveTestData(77-83)saveTestData(85-121)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/fixtures/ProjectApiKeyAuthRequestPerformer.kt (1)
performProjectAuthGet(47-49)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx (2)
webapp/src/views/projects/translations/context/services/useTranslationsService.tsx (1)
useTranslationsService(74-435)webapp/src/service/http/useQueryApi.ts (1)
useApiInfiniteQuery(95-117)
🪛 Biome (2.1.2)
webapp/src/views/projects/translations/context/services/useBranchesService.tsx
[error] 30-30: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Build backend 🏗️
🔇 Additional comments (46)
backend/data/src/main/kotlin/io/tolgee/dtos/IExportParams.kt (2)
22-22: LGTM!The import for
FILTER_BRANCH_DESCRIPTIONis correctly added to support the new schema annotation.
104-107: LGTM!The new
filterBranchproperty is correctly declared with proper nullability and OpenAPI documentation.backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt (1)
137-138: LGTM!The
filterBranchproperty follows the established pattern for filter properties in this class. It's properly annotated with@ActivityLoggedPropfor activity tracking and correctly overridesIExportParams.webapp/src/component/branching/BranchLabel.tsx (1)
18-18: ReconsidertabIndex={0}for a label component.Making a label component focusable with
tabIndex={0}may confuse users, as labels typically aren't interactive. If the component needs to be focusable for a specific reason (e.g., for accessibility with screen readers in a read-only context), consider addingrole="presentation"orrole="text"to clarify its semantic purpose.Verify whether this component is intended to be interactive. If it's purely presentational, remove the
tabIndex.ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchModel.kt (1)
1-20: LGTM!The
BranchModelis well-structured with clear property descriptions. The HATEOAS annotations are properly applied for API documentation.backend/api/src/main/kotlin/io/tolgee/controllers/ExportController.kt (1)
64-70: LGTM!The addition of the
nullparameter togetTranslationsaligns with the branch-aware translation service signature introduced in this PR. Thenullvalue appropriately indicates no branch filtering for this export endpoint.backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/CreateOrUpdateTranslationsFacade.kt (2)
35-35: LGTM!The addition of
dto.branchparameter tokeyService.findcorrectly propagates branch context for key resolution, aligning with the branch-aware architecture introduced in this PR.
77-77: LGTM!The addition of
dto.branchparameter tokeyService.getmaintains consistency with the branch-aware key lookup pattern.backend/data/src/main/kotlin/io/tolgee/events/OnBranchDeleted.kt (1)
1-7: LGTM!The
OnBranchDeletedevent class is simple and correctly structured for event-driven branch cleanup integration.backend/data/src/main/kotlin/io/tolgee/repository/TranslationRepository.kt (1)
25-45: LGTM!The branch filtering logic is correctly implemented:
- When branch parameter is provided: matches only that specific branch (and ensures it's not archived)
- When branch parameter is null: matches keys with no branch association or keys on the default branch
This aligns with the branch-aware filtering pattern used in other queries across the codebase.
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (1)
598-602: LGTM!The
saveBranchesmethod correctly persists unpersisted Branch entities (whereid == 0L) and integrates properly into the project data saving flow.webapp/src/constants/links.tsx (1)
66-66: LGTM!The new routing constants correctly support branch-aware navigation:
TRANSLATIONS_BRANCHparam for branch name in URLsPROJECT_TRANSLATIONS_BRANCHEDfor branched translation viewsPROJECT_BRANCHESfor branch management UIThese align with the broader branching feature implementation.
Also applies to: 352-355, 364-365
backend/data/src/main/kotlin/io/tolgee/service/translation/TranslationService.kt (1)
72-93: LGTM!The branch parameter is correctly threaded through to the repository layer, enabling branch-aware translation retrieval. The implementation properly handles the namespace normalization and passes the branch context to
translationRepository.getTranslations.This aligns with the branch-aware filtering introduced across the data access layer.
backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt (1)
53-53: LGTM!The branch filtering implementation is correct and consistent with the pattern used in other queries:
- When
filterBranchis specified: matches only that specific branch (and ensures it's not archived)- When
filterBranchis null: matches keys with no branch or keys on the default branchThe join initialization and filter invocation are properly integrated into the query construction flow.
Also applies to: 70-70, 97-97, 158-174
webapp/src/component/branching/BranchSelectField.tsx (1)
1-15: LGTM!The Formik integration is correctly implemented:
- Properly uses
useFieldanduseFormikContexthooks- Handles field type conversion (
null→undefined) correctly- Both
onDefaultValueandonSelectcallbacks appropriately set the field value tobranch.idThe component provides clean integration between BranchSelect and Formik form state management.
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchBuilder.kt (1)
5-10: LGTM: simple builder correctly links Branch to Project.backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2KeyController/KeyControllerTest.kt (1)
47-48: Tests correctly validate default vs. branch-scoped key listing.
- Adding branched keys should not affect default listing count (still 123).
- New
returns all keys from branchcovers branch filtering and pagination.Also applies to: 58-59, 71-97
backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
50-51: Enforce DB unique constraint on (namespace_id, name, branch_id)
No existing JPA or migration constraints referencebranch_id. Add or verify a unique constraint coveringnamespace_id,name, andbranch_idin both the@Table(uniqueConstraints=…)of Key.kt and the corresponding DB migration.backend/data/src/main/kotlin/io/tolgee/model/Project.kt (1)
188-194: LGTM: Helper methods correctly handle default branch logic.The
hasDefaultBranch()andgetDefaultBranch()methods provide clean abstractions for checking and retrieving the default branch from the filtered collection.backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt (1)
1-53: LGTM: Well-structured test data builder for branching scenarios.The test data builder correctly sets up:
- A main default protected branch
- A feature branch with origin relationship
- An archived branch to test filtering behavior
- Clean separation with a second project for isolation testing
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/keys/KeyController.kt (2)
128-137: LGTM: Branch parameter correctly added to key listing.The optional
branchparameter enables branch-scoped key retrieval and is properly propagated to the service layer.
155-157: LGTM: Branch name correctly included in key view response.The branch name is safely extracted and included in the
KeyView, enabling clients to display branch context for edited keys.backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (2)
110-133: LGTM: Branch test data covers edge cases including archived branches.The test setup includes:
- A default main branch
- An active dev branch with keys
- An archived dev branch (same name) to verify filtering logic
The hardcoded timestamp for
archivedAt(Jan 1970) is appropriate for testing archived state.
157-173: LGTM: Helper function correctly creates branched keys for testing.The
addNBranchedKeysfunction provides a convenient way to generate test keys within a specific branch, properly linking the branch relationship and setting descriptions.ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/BranchRepository.kt (2)
22-29: LGTM: Query correctly filters by project, branch ID, and archived state.The method appropriately returns a nullable
Branch?and includes the archived filter for consistency.
31-38: LGTM: Case-insensitive branch name lookup is correctly implemented.The query appropriately uses
lower()for case-insensitive matching and filters archived branches.webapp/src/component/branching/BranchSelect.tsx (4)
25-43: LGTM: Branch selection initialization handles both ID and object props.The component correctly resolves the default branch from either an explicit
branchprop (by ID or object match) or falls back to the service'sdefaultBranch.
52-66: LGTM: Item rendering correctly displays branch name and default indicator.The render function appropriately shows a chip for default branches and handles selection state.
68-71: LGTM: Selection handler correctly updates state and notifies parent.
73-91: LGTM: InfiniteSearchSelect configuration is correct for branch selection.The component properly wires up the infinite select with custom branch-specific rendering and labeling.
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerModificationTest.kt (3)
394-413: LGTM: Test correctly verifies branch-specific translation updates.The test properly exercises setting translations for a key within a specific branch and validates the response structure.
415-427: LGTM: Test verifies branch context is required for branched keys.The test correctly ensures that attempting to modify a branched key without providing the branch parameter results in a 404.
429-442: LGTM: Test verifies branch isolation for translation updates.The test correctly ensures that attempting to modify a default-branch key while specifying a different branch results in a 404, maintaining branch isolation.
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt (3)
108-118: LGTM!Test correctly validates that only non-branched/default keys are returned when no branch parameter is specified.
120-131: LGTM!Test correctly validates that both non-branched keys and default branch keys are returned, totaling 7 keys as expected.
133-150: LGTM!Test correctly validates branch-specific filtering and includes thorough translation content verification.
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt (1)
23-122: LGTM!Test suite comprehensively covers branch CRUD operations including edge cases (duplicate names, default branch protection, auto-creation of default branch).
webapp/src/component/searchSelect/InfiniteSearchSelect.tsx (1)
50-76: LGTM!Component enhancements improve flexibility by allowing custom input components, configurable menu positioning, and minimum height control. All props are correctly typed and integrated.
Also applies to: 120-120, 131-131, 135-139, 166-171
backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt (3)
20-32: LGTM!Branch-aware counting and ID pagination methods are correctly implemented.
34-51: LGTM!Branch filtering logic is correct: matches specified non-archived branch or defaults to null/default branch when branch is not specified.
60-71: LGTM!KeyView construction and branch filtering are consistently applied across all retrieval methods.
Also applies to: 154-175, 177-191, 267-277
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (2)
50-76: LGTM!Test thoroughly validates branch copy functionality including key count and key comparison using the helper method.
78-91: LGTM!Performance test ensures branch copy operations scale efficiently with large datasets.
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (1)
67-93: LGTM!Branch parameter is correctly threaded through key lookup, creation, validation, and pagination methods.
Also applies to: 193-222, 291-300, 406-410
webapp/src/service/apiSchema.generated.ts (2)
1349-1363: Branch model semantics — respect isDefault/isProtected and origin selectionUI should:
- Honor isDefault/isProtected when rendering actions (no delete/rename where prohibited).
- Require originBranchId on create (and validate existence).
- Show active status appropriately.
Also applies to: 1932-1940, 3758-3762
13415-13416: No modifications needed — all affected endpoints already includebranch?: stringin their request definitions.
| @Parameter(description = "Branch name to return translations from") | ||
| branch: String? = null, | ||
| request: WebRequest, |
There was a problem hiding this comment.
Annotate branch as a query param to ensure binding and OpenAPI docs.
Without @RequestParam, Spring may not bind it and OpenAPI may not render it as query param.
Apply:
@Parameter(description = "Branch name to return translations from")
- branch: String? = null,
+ @RequestParam(value = "branch", required = false)
+ branch: String? = null,🤖 Prompt for AI Agents
In
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/TranslationsController.kt
around lines 163-165, the branch parameter isn't annotated so Spring won't bind
it as a query parameter and OpenAPI won't display it; annotate the parameter
with @RequestParam(required = false) (and optionally add @Parameter(description
= "Branch name to return translations from")) so it is treated as an optional
query parameter and appears in the generated OpenAPI docs.
| fun generateBranchedData(count: Long, branchName: String = "feature-branch", isBranchDefault: Boolean = false) { | ||
| root.data.projects[0].apply { | ||
| addBranch { | ||
| name = branchName | ||
| project = this@apply.self | ||
| isDefault = isBranchDefault | ||
| }.build { | ||
| (1..count).forEach { | ||
| addKey { | ||
| name = "key from branch $branchName $it" | ||
| branch = this@build.self | ||
| }.build { | ||
| addTranslation { | ||
| language = germanLanguage | ||
| text = "I am key $it's german translation from branch $branchName." | ||
| } | ||
| addTranslation { | ||
| language = englishLanguage | ||
| text = "I am key $it's english translation from branch $branchName." | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Kotlin range type bug: (1..count) with count: Long won’t compile.
Use a LongRange or change the parameter type. The same pattern elsewhere should be aligned.
Option A (keep Long):
- (1..count).forEach {
+ (1L..count).forEach {Option B (prefer Int counts in tests):
- fun generateBranchedData(count: Long, branchName: String = "feature-branch", isBranchDefault: Boolean = false) {
+ fun generateBranchedData(count: Int, branchName: String = "feature-branch", isBranchDefault: Boolean = false) {
...
- (1..count).forEach {
+ (1..count).forEach {🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt
around lines 294-318, the for-range uses (1..count) while count is declared as
Long which doesn't compile; change the parameter type from Long to Int
(preferred for test counts) and update any call sites and other similar methods
in this file to use Int as well so the Kotlin IntRange (1..count) compiles, or
alternatively replace the range with (1L..count).forEach if you must keep Longs
— be consistent across the file.
| cb.or( | ||
| cb.and( | ||
| cb.equalNullable(branch.get(Branch_.name), key.branch), | ||
| cb.isNull(branch.get(Branch_.archivedAt)), | ||
| ), | ||
| cb.or( | ||
| cb.isNull(root.get(Key_.branch)), | ||
| cb.isTrue(branch.get(Branch_.isDefault)), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Fix branch filtering logic to use conditional if/else pattern.
The current OR condition incorrectly includes default branch keys when querying for a specific feature branch. When key.branch is "feature-1", the second condition (cb.isNull(root.get(Key_.branch)) OR cb.isTrue(branch.get(Branch_.isDefault))) would still match default branch keys, mixing results from different branches.
Compare with the correct pattern in ExportDataProvider.kt (lines 158-174) and QueryGlobalFiltering.kt (lines 266-283) which use if/else logic:
- If a branch is specified: match only that specific branch (and not archived)
- Else: match keys with no branch or default branch
Apply this diff to fix the logic:
- cb.or(
- cb.and(
- cb.equalNullable(branch.get(Branch_.name), key.branch),
- cb.isNull(branch.get(Branch_.archivedAt)),
- ),
- cb.or(
- cb.isNull(root.get(Key_.branch)),
- cb.isTrue(branch.get(Branch_.isDefault)),
- ),
- )
+ if (key.branch != null) {
+ cb.and(
+ cb.equal(branch.get(Branch_.name), key.branch),
+ cb.isNull(branch.get(Branch_.archivedAt)),
+ )
+ } else {
+ cb.or(
+ cb.isNull(root.get(Key_.branch)),
+ cb.isTrue(branch.get(Branch_.isDefault)),
+ )
+ }📝 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.
| cb.or( | |
| cb.and( | |
| cb.equalNullable(branch.get(Branch_.name), key.branch), | |
| cb.isNull(branch.get(Branch_.archivedAt)), | |
| ), | |
| cb.or( | |
| cb.isNull(root.get(Key_.branch)), | |
| cb.isTrue(branch.get(Branch_.isDefault)), | |
| ), | |
| ) | |
| if (key.branch != null) { | |
| cb.and( | |
| cb.equal(branch.get(Branch_.name), key.branch), | |
| cb.isNull(branch.get(Branch_.archivedAt)), | |
| ) | |
| } else { | |
| cb.or( | |
| cb.isNull(root.get(Key_.branch)), | |
| cb.isTrue(branch.get(Branch_.isDefault)), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/service/bigMeta/BigMetaService.kt
around lines 238 to 247, the branch filter uses a single cb.or(...) that wrongly
allows default-branch keys to match even when a specific branch is requested;
change it to a conditional if/else: when key.branch is non-null, add only the
AND condition that matches the specific branch name and non-archived branch;
otherwise add the OR condition that matches root Key_.branch IS NULL or
Branch_.isDefault TRUE. Ensure the query builder uses the appropriate branch
predicate depending on the presence of key.branch so specific-branch queries do
not include default-branch keys.
| @Transactional | ||
| fun create(project: Project): Branch { | ||
| val branch = Branch.createMainBranch(project) | ||
| keyService.getAll(project.id).forEach { it -> | ||
| it.branch = branch | ||
| } | ||
| return branch | ||
| } |
There was a problem hiding this comment.
Bug: assigning transient Branch to persisted Keys (unsaved entity). Persist Branch first.
Branch.createMainBranch(project) creates a new entity but it's never saved. Assigning it to existing keys will cause TransientPropertyValueException on flush.
package io.tolgee.ee.service.branching
@@
-import io.tolgee.service.key.KeyService
+import io.tolgee.service.key.KeyService
import io.tolgee.service.project.ProjectService
+import io.tolgee.service.branching.BranchService
@@
class DefaultBranchCreator(
private val keyService: KeyService,
private val projectService: ProjectService,
+ private val branchService: BranchService,
) {
@@
@Transactional
fun create(project: Project): Branch {
- val branch = Branch.createMainBranch(project)
- keyService.getAll(project.id).forEach { it ->
- it.branch = branch
- }
- return branch
+ val created = Branch.createMainBranch(project)
+ val branch = branchService.save(created)
+ // TODO: Consider batch update for performance
+ keyService.getAll(project.id).forEach { key ->
+ key.branch = branch
+ }
+ return branch
}
}Also consider a bulk update method in KeyService/Repository to avoid N updates on large projects.
I can provide a JPQL/SQL batch update snippet if desired.
🤖 Prompt for AI Agents
In
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/DefaultBranchCreator.kt
around lines 22 to 29, the code creates a new Branch with
Branch.createMainBranch(project) but never persists it before assigning it to
existing Key entities, causing a TransientPropertyValueException on flush; fix
by saving the new branch via the Branch repository (or entityManager.persist)
immediately after creation so it has an id and managed state, then assign it to
keys; to avoid N updates for large projects, replace the per-entity loop with a
KeyService/Repository bulk update (JPQL/SQL update setting branch_id where
project_id = :projectId) or add a dedicated batch update method that runs a
single update query and flushes/clears appropriately.
| "/v2/projects/{projectId}/branches": { | ||
| get: operations["all"]; | ||
| post: operations["create_11"]; | ||
| }; | ||
| "/v2/projects/{projectId}/branches/{branchId}": { | ||
| delete: operations["delete_15"]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
New branch endpoints — wire client + UI flows
Add service wrappers and UI for:
- GET/POST /v2/projects/{projectId}/branches (list/create)
- DELETE /v2/projects/{projectId}/branches/{branchId}
Ensure EE gating/permissions, and disable delete for default/protected branches (see schema flags).
Also applies to: 11558-11685
🏁 Script executed:
#!/bin/bash
# Verify client and UI use the new endpoints
rg -nP --type=ts -C2 '/v2/projects/\{?[^}]*\}?/branches\b' || true
rg -nP --type=ts -C2 '\bbranches?\b' webapp | sed -n '1,200p' || trueLength of output: 11688
Enforce EE permissions and disable deletion for protected branches
- Replace
canEditBranches = true(BranchesList.tsx:72) withsatisfiesPermission('branches.edit'). - In BranchesList.tsx, disable the delete action for default/protected branches using the schema’s
protected/isDefaultflag.
🤖 Prompt for AI Agents
In webapp/src/service/apiSchema.generated.ts around lines 413-419 and
BranchesList.tsx around line 72, replace the hardcoded canEditBranches = true
boolean with a runtime permission check using
satisfiesPermission('branches.edit') and update the UI action logic so the
delete action is disabled when the branch schema marks it as protected or
isDefault; specifically, use the branch object's protected or isDefault flag
from the generated schema to conditionally disable or hide the delete button and
ensure permission check gates edit/delete actions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (2)
102-109: Wrap branch deletion assertion in wait loopBranch removal is event-driven; asserting immediately after the key-count wait can still race and flake. Please wrap the branch null check in the same
waitForNotThrowingpattern.- branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() + waitForNotThrowing(timeout = 10000, pollTime = 250) { + branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() + }
171-178: Ensure screenshot lists are non-empty before.first()
mainKeyScreenshots.first()andbranchKeyScreenshots.first()still riskNoSuchElementExceptionif either list is empty. Please add size/non-empty checks before dereferencing.- mainKeyScreenshots.first().let { screenshotReference -> + mainKeyScreenshots.assert.hasSize(branchKeyScreenshots.size) + mainKeyScreenshots.assert.isNotEmpty() + branchKeyScreenshots.assert.isNotEmpty() + mainKeyScreenshots.first().let { screenshotReference ->backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt (1)
290-304: Fix Long range iteration
(1..count)still fails to compile whencountisLong; Kotlin only definesIntRangefor anIntliteral lower bound. Please switch to aLongRange(or changecounttoInt) so this helper builds.- (1..count).forEach { + (1L..count).forEach {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt(4 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/translations/v2TranslationsController/TranslationsControllerViewTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/fixtures/ProjectApiKeyAuthRequestPerformer.kt (1)
performProjectAuthGet(47-49)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/TranslationsTestData.kt (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (6)
addBranch(90-90)addKey(82-82)addKey(105-111)addKey(113-115)addKey(117-128)addTranslation(130-130)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
setDescription(100-103)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addComment(56-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build backend 🏗️
| mainTranslation.comments.assert.hasSize(branchTranslation.comments.size) | ||
| mainTranslation.comments.first().let { comment -> | ||
| val branchComment = branchTranslation.comments.first() | ||
| comment.id.assert.isNotEqualTo(branchComment.id) | ||
| comment.text.assert.isEqualTo(branchComment.text) | ||
| comment.state.assert.isEqualTo(branchComment.state) | ||
| comment.author.assert.isEqualTo(branchComment.author) | ||
| } |
There was a problem hiding this comment.
Guard comment collections before calling .first()
mainTranslation.comments.first() (and the branch counterpart) will throw if the collection is empty. Add explicit size/non-empty assertions before accessing first() so the failure mode stays diagnostic instead of crashing the test.
- mainTranslation.comments.assert.hasSize(branchTranslation.comments.size)
- mainTranslation.comments.first().let { comment ->
+ mainTranslation.comments.assert.hasSize(branchTranslation.comments.size)
+ mainTranslation.comments.assert.isNotEmpty()
+ branchTranslation.comments.assert.isNotEmpty()
+ mainTranslation.comments.first().let { comment ->🤖 Prompt for AI Agents
In
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt
around lines 161 to 168, the test calls mainTranslation.comments.first() and
branchTranslation.comments.first() without guarding against empty collections;
add explicit assertions that the collections are non-empty (or assert their
sizes > 0) before calling first(), or substitute first() usage with safe access
after asserting hasSize/nonEmpty so the test fails with a clear diagnostic
instead of throwing an exception.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (10)
backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt (1)
85-85: Set migration follow-ups (indexed/order-dependent usages).Switching comments to MutableSet breaks index/order assumptions elsewhere; ensure all usages are compatible. See prior review note.
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
11-12: Guard OSS stub with ConditionalOnMissingBean to avoid bean collisions with EE.Without a condition, EE builds may register two
BranchServicebeans and fail startup.Based on learnings
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (3)
108-108: Wrap branch existence check in wait loop to avoid race condition.Deletion is event-driven; the branch existence check at line 108 can race with asynchronous deletion. Wrap it in a
waitForNotThrowingloop similar to the key count check above.Apply this diff:
waitForNotThrowing(timeout = 10000, pollTime = 250) { keyRepository.countByProjectAndBranch( testData.project.id, testData.toBeDeletedBranch.id ).assert.isEqualTo(0) + branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() } - branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() }
164-172: Guard comment collections before calling first().Line 165 calls
first()onmainTranslation.commentswithout verifying the collection is non-empty. If both collections are empty (size check passes),first()will throwNoSuchElementException.Apply this diff:
// translation comments are the same mainTranslation.comments.assert.hasSize(branchTranslation.comments.size) + mainTranslation.comments.assert.isNotEmpty() + branchTranslation.comments.assert.isNotEmpty() mainTranslation.comments.first().let { comment ->
175-182: Add size check before accessing first screenshot reference.Line 177 calls
first()onmainKeyScreenshotswithout verifying the collection is non-empty, which could causeNoSuchElementException.Apply this diff:
// key screenshots are the same val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id)) val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id)) + mainKeyScreenshots.assert.hasSizeGreaterThan(0) + mainKeyScreenshots.assert.hasSize(branchKeyScreenshots.size) mainKeyScreenshots.first().let { screenshotReference ->ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (1)
70-77: Avoid parameter shadowing and return 201 Created on create.Rename local var to avoid shadowing and return 201 for resource creation. (Previously suggested.)
+import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ResponseStatus @@ - fun create(@RequestBody branch: CreateBranchModel): BranchModel { - val branch = branchService.createBranch( + @ResponseStatus(HttpStatus.CREATED) + fun create(@RequestBody payload: CreateBranchModel): BranchModel { + val created = branchService.createBranch( projectHolder.project.id, - branch.name, - branch.originBranchId + payload.name, + payload.originBranchId ) - return branchModelAssembler.toModel(branch) + return branchModelAssembler.toModel(created) }backend/data/src/main/kotlin/io/tolgee/model/branching/Branch.kt (1)
14-15: Use JPA column constraints; remove non‑JPA @DefaultValue; align with schema.
- @DefaultValue from Spring Boot props has no effect on JPA. Remove it.
- name and pending are non‑null in Kotlin; mark nullable = false at column level to match DB and avoid NPEs. This was flagged earlier.
-import org.springframework.boot.context.properties.bind.DefaultValue @@ @ActivityLoggedProp - @Column(length = 200) + @Column(length = 200, nullable = false) var name: String = "", @@ @Column(name = "pending") - var pending: Boolean = false, + var pending: Boolean = false, @@ - @Column(name = "revision") - @DefaultValue("0") + @Column(name = "revision") var revision: Int = 0Note: Keep revision default via field initializer and/or DB default; drop the non‑JPA annotation. Also ensure Liquibase sets NOT NULL/defaults (see schema.xml comment).
Also applies to: 42-61
backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt (1)
288-304: MultipleBagFetchException risk: too many collection fetch‑joins.This query fetch‑joins multiple collections (translations, t.labels, t.comments, km.tags). Hibernate disallows multiple bag fetches; expect row explosion or exceptions. (Previously flagged.)
- Fetch‑join at most one collection (e.g., k.translations), load labels/tags/comments in follow‑up queries, or switch collections to Set and still limit fetch‑joins.
from Key k - left join fetch k.translations t - left join fetch k.keyMeta km - left join fetch k.namespace ns - left join fetch k.branch b - left join fetch t.labels l - left join fetch km.tags tg - left join fetch t.comments c - left join fetch c.author ca + left join fetch k.translations t + left join fetch k.namespace ns + left join fetch k.branch b + left join fetch k.keyMeta kmLoad labels/tags/comments lazily or via dedicated repository methods.
backend/data/src/main/resources/db/changelog/schema.xml (2)
4641-4663: branch.name should be NOT NULL to match Kotlin non‑null field.Add NOT NULL constraint to branch.name (and keep length=200). This prevents nulls at DB and aligns with the entity. Previously raised.
<createTable tableName="branch"> @@ - <column name="name" type="VARCHAR(200)"/> + <column name="name" type="VARCHAR(200)"> + <constraints nullable="false"/> + </column>
4701-4704: branch.pending should be NOT NULL with default false; add backfill to avoid migration failures.Make pending non‑null with default false and backfill existing nulls before applying NOT NULL.
- <addColumn tableName="branch"> - <column name="pending" type="BOOLEAN"/> - </addColumn> + <addColumn tableName="branch"> + <column name="pending" type="BOOLEAN" defaultValueBoolean="false"> + <constraints nullable="false"/> + </column> + </addColumn>Add a safety backfill before enforcing NOT NULL if data may exist:
<changeSet author="db" id="branch-pending-backfill"> <sql>update branch set pending = false where pending is null;</sql> </changeSet>
🧹 Nitpick comments (18)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (3)
168-168: Redundantproject = projectBuilder.selfin addBranch.
addBranchunderProjectBuildershould already attach the project (see ProjectBuilder.addBranch). Remove for consistency.Based on relevant builders.
- project = projectBuilder.self
126-141: Clarify naming for archived dev branch and key
devBranchandfirstDevKeyrefer to the deleted/archived “dev” branch and its first key, which can be confusing given the active “dev” defined above. Consider renaming toarchivedDevBranchandarchivedDevFirstKey(updating usages in KeysTestData, BranchRevisionData, and related tests) or add a short comment explaining they represent the archived entities.
164-180: Optional: make addNBranchedKeys idempotent or return existing branch to avoid duplicate branch creation
Tests only call it once per builder session, so there’s no current duplicate-name issue, but invoking it multiple times on the same project will create duplicate branches. Consider refactoring to reuse an existing non-archived branch or return the created branch.backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeChangeType.kt (1)
3-8: Consider removing unused constructor parameter.The
value: Stringconstructor parameter is not used or exposed. Unless there's a planned use for serialization/deserialization, this can be simplified.Apply this diff to remove the unused parameter:
-enum class BranchKeyMergeChangeType(value: String) { - ADD("ADD"), - UPDATE("UPDATE"), - DELETE("DELETE"), - CONFLICT("CONFLICT"), +enum class BranchKeyMergeChangeType { + ADD, + UPDATE, + DELETE, + CONFLICT, }backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeResolutionType.kt (1)
3-6: Consider removing unused constructor parameter.Similar to
BranchKeyMergeChangeType, thevalue: Stringconstructor parameter is unused.Apply this diff:
-enum class BranchKeyMergeResolutionType(value: String) { - SOURCE("source"), - TARGET("target"), +enum class BranchKeyMergeResolutionType { + SOURCE, + TARGET, }backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
282-282: Optional:cb.literal()may be redundant.The
cb.literal(params.branch)call is likely unnecessary;cb.equal()can handle the string parameter directly.Consider simplifying to:
- cb.equal(branchJoin.get(Branch_.name), cb.literal(params.branch)), + cb.equal(branchJoin.get(Branch_.name), params.branch),ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchMergeModelAssembler.kt (1)
18-21: Optimize change counting with a single pass.The current implementation filters
entity.changesfour times to count each change type separately. This can be optimized to iterate the collection once.Apply this diff to improve performance:
+ val changeCounts = entity.changes.groupingBy { it.change }.eachCount() return BranchMergeModel( id = entity.id, sourceBranch = branchModelAssembler.toModel(entity.sourceBranch), targetBranch = branchModelAssembler.toModel(entity.targetBranch), - keyAdditionsCount = entity.changes.filter { it.change == BranchKeyMergeChangeType.ADD }.size, - keyDeletionsCount = entity.changes.filter { it.change == BranchKeyMergeChangeType.DELETE }.size, - keyModificationsCount = entity.changes.filter { it.change == BranchKeyMergeChangeType.UPDATE }.size, - keyConflictsCount = entity.changes.filter { it.change == BranchKeyMergeChangeType.CONFLICT }.size, + keyAdditionsCount = changeCounts[BranchKeyMergeChangeType.ADD] ?: 0, + keyDeletionsCount = changeCounts[BranchKeyMergeChangeType.DELETE] ?: 0, + keyModificationsCount = changeCounts[BranchKeyMergeChangeType.UPDATE] ?: 0, + keyConflictsCount = changeCounts[BranchKeyMergeChangeType.CONFLICT] ?: 0, )ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchRevisionsTest.kt (1)
66-74: Stabilize async assertions to reduce flakinessPut both assertions inside the polling block and use a slightly longer timeout with shorter poll to avoid races.
- private fun assertBranchMetadataChanged() { - waitForNotThrowing(timeout = 3000, pollTime = 500) { - testData.devBranch.refresh().revision.assert.isEqualTo(1) - } - testData.firstKey.refresh().let { - it.cascadeUpdatedAt.assert.isNotNull - it.cascadeUpdatedAt!!.time.assert.isEqualTo(currentDateProvider.date.time) - } - } + private fun assertBranchMetadataChanged() { + waitForNotThrowing(timeout = 5000, pollTime = 100) { + testData.devBranch.refresh().revision.assert.isEqualTo(1) + testData.firstKey.refresh().let { + it.cascadeUpdatedAt.assert.isNotNull + it.cascadeUpdatedAt!!.time.assert.isEqualTo(currentDateProvider.date.time) + } + } + }Also applies to: 39-40
backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt (1)
22-30: Add cascade to changes and drop redundant targetEntity attributesCascading simplifies persistence/removal of related changes and
targetEntityis unnecessary when using typed properties.- @ManyToOne(targetEntity = Branch::class) + @ManyToOne @JoinColumn(name = "source_branch_id", nullable = false) @NotNull lateinit var sourceBranch: Branch - @ManyToOne(targetEntity = Branch::class) + @ManyToOne @JoinColumn(name = "target_branch_id", nullable = false) @NotNull lateinit var targetBranch: Branch- @OneToMany(targetEntity = BranchMergeChange::class, orphanRemoval = true, mappedBy = "branchMerge") + @OneToMany(mappedBy = "branchMerge", orphanRemoval = true, cascade = [CascadeType.ALL]) var changes: MutableList<BranchMergeChange> = mutableListOf()Add missing import:
import jakarta.persistence.CascadeTypeAlso applies to: 38-39
backend/data/src/main/kotlin/io/tolgee/model/branching/BranchVersionedEntity.kt (1)
12-17: Fix KDoc (key vs branch) and allow nullable values in oldStateDocs currently say “branch” but method resolves key id. Also, old state may contain nulls; widen to
Any?.- /** - * Resolves the unique identifier of the branch associated with the entity in the branching model. - * - * @return The ID of the branch - */ + /** + * Resolves the unique identifier of the key associated with the entity in the branching model. + * + * @return The ID of the key + */ fun resolveKeyId(): Long?- /** - * Determines if an entity has relevant changes in data in terms of branch versioning. - * - * @param oldState The original object data to compare. - * @return True if the update actually modified data - */ - fun isModified(oldState: Map<String, Any>): Boolean = true + /** + * Determines if an entity has relevant changes in data in terms of branch versioning. + * + * @param oldState Previous values (may contain nulls). + * @return True if the update actually modified data + */ + fun isModified(oldState: Map<String, Any?>): Boolean = trueIf you apply this signature change, update implementers accordingly (Key, Translation, KeyMeta). I can prepare a follow-up diff.
Also applies to: 19-26
backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
103-116: Avoid sharing mutable map instances and alignisModifiedwith nullable state
- Deep-copy
customto prevent aliasing between entities.- If
BranchVersionedEntity.isModifiedswitches toMap<String, Any?>, reflect it here.- override fun isModified(oldState: Map<String, Any>): Boolean { + override fun isModified(oldState: Map<String, Any?>): Boolean { return oldState["description"] != this.description || oldState["custom"] != this.custom } @@ override fun merge(source: KeyMeta) { this.description = source.description - this.custom = source.custom + this.custom = source.custom?.toMutableMap() }ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchModelAssembler.kt (1)
10-18: Add HATEOAS links and handle BranchModel.id nullability
- BranchModel.id is declared as non-nullable Long; entity.id can be null before persistence—use Long? or enforce assembler use only on persisted entities
- Enrich BranchModel with HATEOAS links for self, project, and merges
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeModel.kt (1)
8-9: Add collectionRelation for proper HATEOAS collection rendering.Define collection name for list endpoints.
-@Relation(itemRelation = "branchMerge") +@Relation(itemRelation = "branchMerge", collectionRelation = "branchMerges")ee/backend/app/src/main/kotlin/io/tolgee/ee/component/branching/BranchContentEventListener.kt (1)
12-13: Rename field to match type.branchRevisionUpdater → branchMetaUpdater for consistency with BranchMetaUpdater.
-class BranchContentEventListener( - private val branchRevisionUpdater: BranchMetaUpdater, +class BranchContentEventListener( + private val branchMetaUpdater: BranchMetaUpdater, ) {Also update the call site on Line 42 accordingly.
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt (1)
88-99: Semantics check: DELETE of target-only keys.Confirm intended behavior: merging source→target deletes keys that exist only in target. If not desired, adjust to NOOP.
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
55-57: Redundant project ID check after repository query.The
findByProjectIdAndIdquery at line 51 already filters byprojectId, so checkingoriginBranch.project.id != projectIdat line 55 is redundant. The check can never fail.Consider removing the redundant check:
val originBranch = branchRepository.findByProjectIdAndId(projectId, originBranchId) ?: throw BadRequestException( Message.ORIGIN_BRANCH_NOT_FOUND ) - - if (originBranch.project.id != projectId) { - throw BadRequestException(Message.ORIGIN_BRANCH_NOT_FOUND) - } val branch = createBranch(projectId, name).also {
85-91: Explicitly save the branch after setting archivedAt.While JPA dirty checking will persist the change, explicitly calling
branchRepository.save(branch)after line 89 makes the intent clearer and is more maintainable.Apply this diff:
if (branch.isDefault) throw PermissionException(Message.CANNOT_DELETE_DEFAULT_BRANCH) branch.archivedAt = currentDateProvider.date + branchRepository.save(branch) applicationContext.publishEvent(OnBranchDeleted(branch)) }backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
146-153: differsInBranchVersion() always true — verify intent.Always returning true can cause every key version to be treated as diverging across branches, increasing merge noise. Consider comparing actual fields or confirm this is intentional.
- override fun differsInBranchVersion(entity: Key): Boolean { - return true - } + override fun differsInBranchVersion(entity: Key): Boolean { + return this.isPlural != entity.isPlural || + this.pluralArgName != entity.pluralArgName + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchRevisionData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/DryRunMergeBranchRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/MergeBranchRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/Branch.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMergeChange.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchVersionedEntity.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeChangeType.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeResolutionType.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt(9 hunks)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt(9 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt(3 hunks)backend/data/src/main/resources/db/changelog/schema.xml(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchMergeModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/component/branching/BranchContentEventListener.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/component/branching/BranchMetaUpdater.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchRevisionsTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCopyServiceSql.kt
🧰 Additional context used
🧬 Code graph analysis (4)
backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (1)
backend/data/src/main/kotlin/io/tolgee/service/export/dataProvider/ExportDataProvider.kt (1)
filterBranch(158-174)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (2)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addBranch(90-90)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (2)
addMeta(44-47)setDescription(100-103)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchRevisionData.kt (5)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/TestDataBuilder.kt (2)
addUserAccount(36-51)addProject(53-61)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (4)
addLanguage(80-80)addPermission(74-74)addBranch(90-90)addScreenshotReference(144-144)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt (1)
addLabel(132-135)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
addMeta(44-47)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (2)
addComment(57-65)addCodeReference(67-75)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (2)
createBranch(12-12)getBranch(11-11)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (2)
createBranch(21-23)getBranch(17-19)
🪛 GitHub Actions: Test
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt
[error] 29-29: mergeBranch overrides nothing. Kotlin compilation error.
🔇 Additional comments (7)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt (2)
113-117: Default “main” branch setup looks good.Marks the default branch explicitly; aligns with branching features.
107-108: LGTM on author assignment.
author = useruses the user created above; consistent with the test data.backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryGlobalFiltering.kt (2)
269-271: Clarify the early return logic forfilterKeyId.The early return when
filterKeyIdis not empty bypasses all branch filtering. This means specific key queries ignore branch constraints entirely, which might be intentional but could be surprising.Please confirm this is the desired behavior, as it allows queries to retrieve keys from any branch (including archived branches) when filtering by key ID.
272-287: Branch filtering logic looks correct.The branch filtering implementation correctly handles:
- Default behavior: includes keys with null branches or default branch
- Named branch: filters by branch name and excludes archived branches
This aligns with the similar logic in
ExportDataProvider.filterBranch().backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/DryRunMergeBranchRequest.kt (1)
6-8: LGTM!The DTO is well-structured with clear, consistent naming between the property (
targetBranchId) and its description.backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (1)
9-15: LGTM!The interface is well-designed with clear method signatures for branch lifecycle management (CRUD operations and dry-run merge preview).
backend/data/src/main/kotlin/io/tolgee/repository/KeyRepository.kt (1)
60-71: Verify broadened scope: includes keys from all non‑archived branches.This projection now returns keys where (br is null or br.archivedAt is null). That includes all non‑archived branches, not just default. Confirm callers expect branched keys in this endpoint.
| addKey { | ||
| name = "first_key" | ||
| branch = self | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Disambiguate self to avoid receiver mix-ups inside nested builders.
In these spots, self can resolve to the inner Key builder, not the outer Branch, causing type mismatch or fragile resolution. Capture the branch first and use that variable.
Apply these diffs:
@@
- }.build {
- addKey {
- name = "first_key"
- branch = self
- }
+ }.build {
+ val branchSelf = self
+ addKey {
+ name = "first_key"
+ branch = branchSelf
+ }
}@@
- }.build {
- firstDevKey = addKey {
- name = "first_key"
- branch = self
- }.build {
+ }.build {
+ val branchSelf = self
+ firstDevKey = addKey {
+ name = "first_key"
+ branch = branchSelf
+ }.build {
addMeta {
description = "default"
}
}.self
}.self@@
- }.build {
- (1..n).forEach {
+ }.build {
+ val branchSelf = self
+ (1..n).forEach {
addKey {
name = "branch_key_$it"
- this.branch = self
+ this.branch = branchSelf
}.build {
setDescription("description of branched key")
}
}
}Also applies to: 132-135, 171-174
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt
around lines 121-124 (also update similar spots at 132-135 and 171-174), the
nested builder uses an unqualified self which can resolve to the inner Key
builder instead of the outer Branch; capture the branch into a local val before
entering the inner addKey block and reference that local (e.g., val branchRef =
self) when setting branch = branchRef so the correct Branch instance is used and
receiver resolution is unambiguous.
| // deleted branch with the same name | ||
| devBranch = addBranch { | ||
| name = "dev" | ||
| archivedAt = Date(1759833439) |
There was a problem hiding this comment.
Date(1759833439) uses milliseconds; likely intended epoch seconds. Prefer Instant.
Use an explicit epoch-seconds conversion or ISO instant to avoid magic numbers.
- archivedAt = Date(1759833439)
+ archivedAt = Date.from(java.time.Instant.ofEpochSecond(1_759_833_439L))Also add:
import java.time.Instantoutside this hunk.
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/KeysTestData.kt
around line 130, Date(1759833439) is using the value as milliseconds but that
literal is an epoch-seconds timestamp; replace it with
Date.from(Instant.ofEpochSecond(1759833439)) (or use an ISO instant via
Instant.parse if preferred) to ensure correct epoch-seconds conversion, and add
the import line "import java.time.Instant" at the top of the file.
| @Schema(description = "Target branch id") | ||
| val mergeSessionId: Long, |
There was a problem hiding this comment.
Clarify property naming: mergeSessionId vs "Target branch id".
The property name mergeSessionId suggests a session identifier, but the schema description says "Target branch id". This naming mismatch can confuse API consumers.
Consider either:
- Renaming the property to
targetBranchIdfor consistency with the description (and DryRunMergeBranchRequest) - Updating the description to accurately reflect what a "merge session" represents
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/MergeBranchRequest.kt
around lines 7-8, the property name mergeSessionId conflicts with the Schema
description "Target branch id"; rename the property to targetBranchId to match
the description and align with DryRunMergeBranchRequest (update constructor
param, usages, serializers/deserializers and tests), or alternatively update the
@Schema description to accurately describe that the field is a merge session
identifier if you intend to keep the current name; ensure DTOs, API docs and any
mapping code are updated consistently.
| @ManyToOne(targetEntity = BranchMerge::class) | ||
| @JoinColumn(name = "branch_merge_id", nullable = false) | ||
| lateinit var branchMerge: BranchMerge | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "source_key_id", nullable = true) | ||
| lateinit var sourceKey: Key | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "target_key_id", nullable = true) | ||
| lateinit var targetKey: Key |
There was a problem hiding this comment.
Fix nullability and fetch types on associations.
DB columns are nullable; Kotlin fields must be nullable. Also prefer LAZY for branchMerge to avoid eager graph loading.
- @ManyToOne(targetEntity = BranchMerge::class)
+ @ManyToOne(fetch = FetchType.LAZY, targetEntity = BranchMerge::class)
@JoinColumn(name = "branch_merge_id", nullable = false)
lateinit var branchMerge: BranchMerge
- @ManyToOne(fetch = FetchType.LAZY)
- @JoinColumn(name = "source_key_id", nullable = true)
- lateinit var sourceKey: Key
+ @ManyToOne(fetch = FetchType.LAZY)
+ @JoinColumn(name = "source_key_id", nullable = true)
+ var sourceKey: Key? = null
- @ManyToOne(fetch = FetchType.LAZY)
- @JoinColumn(name = "target_key_id", nullable = true)
- lateinit var targetKey: Key
+ @ManyToOne(fetch = FetchType.LAZY)
+ @JoinColumn(name = "target_key_id", nullable = true)
+ var targetKey: Key? = null
@Column(nullable = false)
@Enumerated(EnumType.STRING)
lateinit var change: BranchKeyMergeChangeTypeConsider renaming property “change” to “type” to reduce naming collisions (optional).
Also applies to: 48-55
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMergeChange.kt
around lines 36-46 (and also apply the same fixes to lines 48-55), the entity
associations are declared as non-null and some are eagerly fetched: change the
Kotlin properties to be nullable (use Key? and BranchMerge?) to match nullable
DB columns instead of lateinit, set fetch = FetchType.LAZY on the branchMerge
@ManyToOne as well, and keep @JoinColumn(nullable = true) where the DB allows
nulls; optionally rename the property “change” to “type” to avoid collisions if
desired.
| val modifiedAt: Date | ||
| get() = cascadeUpdatedAt ?: updatedAt!! | ||
|
|
There was a problem hiding this comment.
Avoid potential NPE in modifiedAt (updatedAt!!).
modifiedAt dereferences updatedAt!!, which can be null pre‑persist. Use a safe fallback.
- val modifiedAt: Date
- get() = cascadeUpdatedAt ?: updatedAt!!
+ val modifiedAt: Date
+ get() = cascadeUpdatedAt ?: updatedAt ?: createdAt ?: Date()📝 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.
| val modifiedAt: Date | |
| get() = cascadeUpdatedAt ?: updatedAt!! | |
| val modifiedAt: Date | |
| get() = cascadeUpdatedAt ?: updatedAt ?: createdAt ?: Date() |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt around lines 111-113,
modifiedAt currently force-unwraps updatedAt (updatedAt!!), risking an NPE
pre-persist; change the getter to avoid the !! by returning cascadeUpdatedAt if
non-null, otherwise updatedAt if non-null, otherwise a safe fallback (e.g. new
Date() or a fixed epoch Date) so the property never throws — update the
expression accordingly and ensure imports/semantics remain consistent.
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt
Outdated
Show resolved
Hide resolved
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeModel.kt
Outdated
Show resolved
Hide resolved
| onChange( | ||
| event.entity, | ||
| event.previousState?.let { | ||
| event.propertyNames?.zip(it) | ||
| }?.toMap() | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use nullable types in oldState map and tighten check.
Event arrays may contain nulls and not include collections. Also, prefer Any? in the map type.
- onChange(
+ onChange(
event.entity,
- event.previousState?.let {
- event.propertyNames?.zip(it)
- }?.toMap()
+ event.previousState?.let { prev ->
+ event.propertyNames?.zip(prev)
+ }?.toMap()
)- fun onChange(entity: Any?, oldState: Map<String, Any>? = null) {
+ fun onChange(entity: Any?, oldState: Map<String, Any?>? = null) {
if (entity == null) return
if (entity !is BranchVersionedEntity<*>) return
val keyId = entity.resolveKeyId() ?: return
- if (oldState == null || entity.isModified(oldState)) {
- branchRevisionUpdater.snapshot(keyId)
+ if (oldState == null || entity.isModified(oldState)) {
+ branchMetaUpdater.snapshot(keyId)
}
}Also applies to: 35-44
🤖 Prompt for AI Agents
In
ee/backend/app/src/main/kotlin/io/tolgee/ee/component/branching/BranchContentEventListener.kt
around lines 27-33 (and similarly 35-44), the code builds a Map from
event.propertyNames and event.previousState but uses non-nullable value types
and assumes previousState contains non-null collection elements; change the map
type to Map<String, Any?> and construct it safely so nulls in previousState are
preserved and missing collections are handled: use
event.propertyNames?.zip(event.previousState ?: emptyArray<Any?>())?.mapValues {
it.value as Any? }?.toMap() (or build via indices to align lengths), and ensure
downstream onChange signature accepts Map<String, Any?>; tighten null checks
accordingly so no unsafe casts or assumptions about array contents remain.
| @Async | ||
| @Transactional | ||
| fun snapshot(keyId: Long) { | ||
| val key = keyRepository.findByIdWithBranch(keyId) ?: return | ||
| key.branch!!.revision++ | ||
| key.cascadeUpdatedAt = currentDateProvider.date | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Review async + transactional semantics and null-safety.
This method combines @Async and @Transactional, which can lead to unexpected behavior:
-
Transaction boundary concern: The async method may execute after the calling transaction commits, potentially causing the incremented revision to be lost if the outer transaction rolls back, or accessing stale/detached entities.
-
Null-pointer risk (Line 19):
key.branch!!uses the non-null assertion operator, but there's no guarantee thatbranchis non-null afterfindByIdWithBranch. If a key exists without an associated branch, this will throw a NPE.
Recommendations:
- Verify that
findByIdWithBranchguarantees a non-null branch, or replace!!with safe-call + early return:key.branch?.let { ... } ?: return - Consider whether async execution is necessary here. If revision tracking must be synchronous with the key update, remove
@Async. - If async is required, ensure proper transaction propagation (e.g.,
@Transactional(propagation = Propagation.REQUIRES_NEW)) and add error handling.
Run this script to check if findByIdWithBranch can return keys with null branches:
🏁 Script executed:
#!/bin/bash
# Search for findByIdWithBranch implementation and usages
rg -nP --type=kt -A10 'fun findByIdWithBranch'Length of output: 26
🏁 Script executed:
#!/bin/bash
echo "=== findByIdWithBranch definition/usages ==="
rg -n 'findByIdWithBranch' -C5 || echo "(none found)"
echo -e "\n=== KeyRepository files ==="
fd KeyRepository.kt || echo "(none found)"
echo -e "\n=== Key entity definition ==="
rg -n 'class Key' -C5 || echo "(none found)"Length of output: 111114
🏁 Script executed:
#!/bin/bash
echo "=== branch property in Key entity ==="
rg -nP '@ManyToOne.*branch' -C3 backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt || echo "(branch mapping not found)"Length of output: 64
🏁 Script executed:
sed -n '1,100p' backend/data/src/main/kotlin/io/tolgee/model/key/Key.ktLength of output: 3389
Fix null assertion and transaction semantics in snapshot()
- Line 19: replace
key.branch!!with a safe call and early return (e.g.key.branch?.apply { revision++ } ?: return) sincebranchis nullable. - Evaluate
@Asyncusage: if order matters, remove it; if retained, use@Transactional(propagation = Propagation.REQUIRES_NEW)and add error handling.
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt (3)
102-108: Wait for branch deletion before asserting.
Directly asserting the branch removal can run ahead of the async cleanup and make the test flaky. Reuse the same wait helper you already employ for the key count to keep the assertion reliable.- branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() + waitForNotThrowing(timeout = 10000, pollTime = 250) { + branchRepository.findByIdOrNull(testData.toBeDeletedBranch.id).assert.isNull() + }
164-172: Guard comments before callingfirst().
Without an explicit non-empty assertion,.first()blows up when a translation has no comments, turning this into an NPE instead of a useful test failure.mainTranslation.comments.assert.hasSize(branchTranslation.comments.size) - mainTranslation.comments.first().let { comment -> - val branchComment = branchTranslation.comments.first() + mainTranslation.comments.assert.isNotEmpty() + branchTranslation.comments.assert.isNotEmpty() + mainTranslation.comments.first().let { comment -> + val branchComment = branchTranslation.comments.first()
175-182: Check screenshot collections before usingfirst().
first()on an empty screenshot list yields aNoSuchElementException, so add the missing size/non-empty assertions before dereferencing.val mainKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(this.id)) val branchKeyScreenshots = keyScreenshotReferenceRepository.getAllByKeyIdIn(listOf(other.id)) - mainKeyScreenshots.first().let { screenshotReference -> - val branchScreenshotReference = branchKeyScreenshots.first() + mainKeyScreenshots.assert.hasSizeGreaterThan(0) + mainKeyScreenshots.assert.hasSize(branchKeyScreenshots.size) + branchKeyScreenshots.assert.hasSizeGreaterThan(0) + mainKeyScreenshots.first().let { screenshotReference -> + val branchScreenshotReference = branchKeyScreenshots.first()ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchRepository.kt (1)
12-21: Search filter missing and nullablePageablebreaks paging guarantees.
searchis never applied, so callers cannot filter branches by name, and keepingPageablenullable invites null usage even though the query always returns aPage. This mirrors earlier feedback and still needs fixing. Please wire thesearchpredicate directly into the JPQL and makepagenon-null to match actual usage. citepast_review_comments@Query( """ select b from Branch b - where b.project.id = :projectId and b.archivedAt IS NULL + where b.project.id = :projectId + and b.archivedAt IS NULL + and (:search is null or lower(b.name) like lower(concat('%', :search, '%'))) order by b.isDefault desc, b.createdAt desc, b.id desc """ ) - fun getAllProjectBranches(projectId: Long, page: Pageable?, search: String?): Page<Branch> + fun getAllProjectBranches(projectId: Long, page: Pageable, search: String?): Page<Branch>ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCleanupService.kt (2)
43-44: Handle missing branch gracefully instead of calling.get().
findById(branchId).get()will throw if the branch disappears before cleanup runs asynchronously. Return early when the branch is absent to avoidNoSuchElementException. citepast_review_comments- val branch = branchRepository.findById(branchId).get() + val branch = branchRepository.findById(branchId).orElse(null) ?: return
47-64: Reset pagination each batch to avoid skipping deletions.Incrementing the page index after every delete skips records because offsets shift once rows are removed. Always fetch page 0 so every remaining batch is processed, just as flagged previously. citepast_review_comments
- var page = 0 while (true) { val idsPage = keyRepository.findIdsByProjectAndBranch( branch.project.id, branch.id, - PageRequest.of(page, BATCH_SIZE) + PageRequest.of(0, BATCH_SIZE) ) if (idsPage.isEmpty) break val ids = idsPage.content if (ids.isNotEmpty()) { keyService.deleteMultiple(ids) logger.debug("Deleted ${ids.size} keys for branch ${branch.id}") } if (idsPage.numberOfElements < BATCH_SIZE) { break } - page++ }backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
12-13: Add @ConditionalOnMissingBean to prevent bean collision.Without
@ConditionalOnMissingBean(BranchService::class), the OSS stub and EE implementation will both register asBranchServicebeans. While@Primaryon the EE impl resolves injection, this pattern is cleaner.Apply this diff:
import org.springframework.data.domain.Page import org.springframework.data.domain.Pageable import org.springframework.stereotype.Service +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean @Service +@ConditionalOnMissingBean(BranchService::class) class BranchServiceOssStub : BranchService {ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (1)
76-82: Avoid parameter shadowing; return 201 Created.The parameter
branchis shadowed by the local variable on line 77, reducing readability. Additionally, resource creation should return HTTP 201 per REST conventions.Apply this diff:
+import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ResponseStatus - fun create(@RequestBody branch: CreateBranchModel): BranchModel { - val branch = branchService.createBranch( + @ResponseStatus(HttpStatus.CREATED) + fun create(@RequestBody payload: CreateBranchModel): BranchModel { + val created = branchService.createBranch( projectHolder.project.id, - branch.name, - branch.originBranchId + payload.name, + payload.originBranchId ) - return branchModelAssembler.toModel(branch) + return branchModelAssembler.toModel(created) }
🧹 Nitpick comments (5)
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (2)
85-92: Consider returning 204 No Content for DELETE.Per REST conventions, successful DELETE operations should return HTTP 204 No Content.
Apply this diff:
+import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ResponseStatus + @ResponseStatus(HttpStatus.NO_CONTENT) fun delete(@PathVariable branchId: Long) {
38-38: Consider restricting CORS origins in production.The wildcard
origins = ["*"]allows any origin. While authentication/authorization protect these endpoints, consider configuring specific allowed origins for production deployments.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
65-67: Remove redundant project ID check.Line 61's
findByProjectIdAndId(projectId, originBranchId)already filters byprojectId, so if it returns a branch, that branch must belong to the project. The check on lines 65-67 is redundant.Apply this diff:
val originBranch = branchRepository.findByProjectIdAndId(projectId, originBranchId) ?: throw BadRequestException( Message.ORIGIN_BRANCH_NOT_FOUND ) - if (originBranch.project.id != projectId) { - throw BadRequestException(Message.ORIGIN_BRANCH_NOT_FOUND) - } - val branch = createBranch(projectId, name).also {
95-101: Add repository save after archiving the branch.Line 99 sets
archivedAtbut doesn't explicitly save. While JPA dirty checking may persist this in the transaction, explicitly saving improves clarity and ensures persistence.Apply this diff:
if (branch.isDefault) throw PermissionException(Message.CANNOT_DELETE_DEFAULT_BRANCH) branch.archivedAt = currentDateProvider.date + branchRepository.save(branch) applicationContext.publishEvent(OnBranchDeleted(branch)) }backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
18-45: Add descriptive messages to UnsupportedOperationException.Providing a message like
"Branching is available in EE only"improves the developer experience when these methods are called in OSS builds.Apply this diff:
override fun getBranch(projectId: Long, branchId: Long): Branch { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun createBranch(projectId: Long, name: String, originBranchId: Long): Branch { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun deleteBranch(projectId: Long, branchId: Long) { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun dryRunMergeBranch(projectId: Long, branchId: Long, request: DryRunMergeBranchRequest): BranchMerge { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun getBranchMerge(projectId: Long, branchId: Long, branchMergeId: Long): BranchMergeView { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun getBranchMergeConflicts( projectId: Long, branchId: Long, branchMergeId: Long, pageable: Pageable ): Page<BranchMergeConflictView> { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/branching/BranchMergeConflictView.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/branching/BranchMergeView.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeChangeType.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchMergeConflictModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchMergeModelAssembler.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeConflictModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeRefModel.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchCleanupService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchCopyIntegrationTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchRevisionsTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/model/branching/BranchMergeModel.kt
- ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchRevisionsTest.kt
- backend/data/src/main/kotlin/io/tolgee/constants/Message.kt
- backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt
🧰 Additional context used
🧬 Code graph analysis (2)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (2)
createBranch(14-14)getBranch(13-13)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (2)
createBranch(22-24)getBranch(18-20)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt (4)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/TestDataBuilder.kt (1)
addProject(53-61)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (6)
addKey(82-82)addKey(105-111)addKey(113-115)addKey(117-128)addBranch(90-90)addTranslation(130-130)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
addMeta(44-47)backend/data/src/main/kotlin/io/tolgee/model/key/KeyMeta.kt (1)
addComment(57-65)
🔇 Additional comments (9)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt (3)
10-27: LGTM! Clean structure and organization.The class structure is well-organized with clear separation of concerns. All
lateinitproperties are properly initialized within the init block.
29-37: LGTM!The helper correctly creates a project without explicit branches for testing default branch creation scenarios.
68-94: No issues found. The shorthandaddTranslation(languageTag: String, text: String?)method exists in KeyBuilder and is correctly used at lines 73 and 86.The code is valid. Lines 73 and 86 in BranchTestData.kt properly call the shorthand overload defined in KeyBuilder.kt (lines 90-97), which accepts a language tag and translation text and delegates to the lambda-based variant.
Likely an incorrect or invalid review comment.
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (3)
56-69: LGTM!The endpoint correctly implements paginated branch listing with optional search. Security and pagination handling are appropriate.
106-117: LGTM!The dry-run merge endpoint correctly delegates to the service layer and returns a merge reference for later retrieval.
119-145: LGTM!The merge preview and conflicts endpoints correctly implement retrieval with proper pagination and HATEOAS assembly.
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (3)
83-93: LGTM!The helper method correctly validates uniqueness and efficiently creates a new branch entity using
getReference.
103-108: LGTM!The dry-run merge correctly validates and delegates to the merge service within a transaction.
45-52: ****The default branch IS persisted. When
DefaultBranchCreator.create()updates managed JPA entities (Key objects) to reference the new Branch, Hibernate automatically flushes the new Branch to the database as part of the transaction. The testBranchControllerTest.creates default branch on project without branchesverifies this by explicitly retrieving the persisted branch viabranchRepository.findByIdOrNull(it)!!, confirming the branch exists in the database after the firstgetAllBranches()call. No explicit save is required—this is standard JPA behavior for managed entity relationships.Likely an incorrect or invalid review comment.
| private fun ProjectBuilder.addBranches() { | ||
| addBranch { | ||
| name = "main" | ||
| project = projectBuilder.self | ||
| isProtected = true | ||
| isDefault = true | ||
| createdAt = currentDateProvider.date | ||
| }.build { | ||
| mainBranch = self | ||
| addBranch { | ||
| name = "feature-branch" | ||
| project = projectBuilder.self | ||
| isProtected = false | ||
| isDefault = false | ||
| originBranch = this | ||
| }.build { | ||
| featureBranch = self | ||
| } | ||
| addBranch { | ||
| name = "merged-and-deleted-branch" | ||
| project = projectBuilder.self | ||
| isProtected = false | ||
| isDefault = false | ||
| archivedAt = currentDateProvider.date.addDays(-1) | ||
| originBranch = this | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: originBranch = this creates self-references instead of parent references.
On lines 53 and 63, originBranch = this is incorrect. Inside the addBranch { ... } lambda, this refers to the Branch being configured (feature-branch and merged-and-deleted-branch respectively), not the main branch. This creates self-referential origins instead of proper parent-child relationships.
Apply this diff to fix the origin branch references:
addBranch {
name = "feature-branch"
project = projectBuilder.self
isProtected = false
isDefault = false
- originBranch = this
+ originBranch = mainBranch
}.build {
featureBranch = self
}
addBranch {
name = "merged-and-deleted-branch"
project = projectBuilder.self
isProtected = false
isDefault = false
archivedAt = currentDateProvider.date.addDays(-1)
- originBranch = this
+ originBranch = mainBranch
}🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt
around lines 39 to 66, the lambdas set originBranch = this which makes the
branch reference itself; change those assignments so the child branches set
originBranch to the parent/main branch (e.g., use the outer/main branch variable
or reference created earlier such as mainBranch or the built parent branch)
instead of `this`, ensuring the feature-branch and merged-and-deleted-branch
correctly reference the main branch as their origin.
| enum class BranchKeyMergeChangeType(value: String) { | ||
| ADD("ADD"), | ||
| UPDATE("UPDATE"), | ||
| DELETE("DELETE"), | ||
| CONFLICT("CONFLICT"), | ||
| SKIP("SKIP"), | ||
| } |
There was a problem hiding this comment.
Declare the value parameter as a property or remove it.
The enum constructor parameter value is not declared as a property (val or var), making it inaccessible. This is likely unintentional.
Solution 1: If you need a string property (e.g., for serialization), declare it as val:
-enum class BranchKeyMergeChangeType(value: String) {
+enum class BranchKeyMergeChangeType(val value: String) {
ADD("ADD"),
UPDATE("UPDATE"),
DELETE("DELETE"),
CONFLICT("CONFLICT"),
SKIP("SKIP"),
}Solution 2 (recommended): If string values aren't needed, simplify by removing the parameter:
Since the string values match the enum names exactly and Kotlin enums already have a built-in .name property, you can simplify to:
-enum class BranchKeyMergeChangeType(value: String) {
- ADD("ADD"),
- UPDATE("UPDATE"),
- DELETE("DELETE"),
- CONFLICT("CONFLICT"),
- SKIP("SKIP"),
+enum class BranchKeyMergeChangeType {
+ ADD,
+ UPDATE,
+ DELETE,
+ CONFLICT,
+ SKIP,
}📝 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.
| enum class BranchKeyMergeChangeType(value: String) { | |
| ADD("ADD"), | |
| UPDATE("UPDATE"), | |
| DELETE("DELETE"), | |
| CONFLICT("CONFLICT"), | |
| SKIP("SKIP"), | |
| } | |
| enum class BranchKeyMergeChangeType(val value: String) { | |
| ADD("ADD"), | |
| UPDATE("UPDATE"), | |
| DELETE("DELETE"), | |
| CONFLICT("CONFLICT"), | |
| SKIP("SKIP"), | |
| } |
| enum class BranchKeyMergeChangeType(value: String) { | |
| ADD("ADD"), | |
| UPDATE("UPDATE"), | |
| DELETE("DELETE"), | |
| CONFLICT("CONFLICT"), | |
| SKIP("SKIP"), | |
| } | |
| enum class BranchKeyMergeChangeType { | |
| ADD, | |
| UPDATE, | |
| DELETE, | |
| CONFLICT, | |
| SKIP, | |
| } |
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/model/enums/BranchKeyMergeChangeType.kt
around lines 3 to 9, the enum declares a constructor parameter `value: String`
but doesn't expose it as a property (missing `val`/`var`) which makes it
inaccessible; either (A) if you need a per-constant string, change the parameter
to a property by declaring `val value: String` in the constructor, or (B) if you
don't need a separate string (since names match), remove the constructor
parameter entirely and simplify the enum to only the constants.
| keyAdditionsCount = entity.keyAdditionsCount.toInt(), | ||
| keyDeletionsCount = entity.keyDeletionsCount.toInt(), | ||
| keyModificationsCount = entity.keyModificationsCount.toInt(), | ||
| keyConflictsCount = entity.keyConflictsCount.toInt(), |
There was a problem hiding this comment.
Don’t downcast merge counters.
BranchMergeView already exposes the counts as Long because the SQL COUNT(*) returns BIGINT. Narrowing them to Int will silently overflow once a project accumulates more than ~2.1 billion changes, producing bogus API responses. Keep the public model on Long (and adjust BranchMergeModel to match) so you don’t lose data.
- keyAdditionsCount = entity.keyAdditionsCount.toInt(),
- keyDeletionsCount = entity.keyDeletionsCount.toInt(),
- keyModificationsCount = entity.keyModificationsCount.toInt(),
- keyConflictsCount = entity.keyConflictsCount.toInt(),
+ keyAdditionsCount = entity.keyAdditionsCount,
+ keyDeletionsCount = entity.keyDeletionsCount,
+ keyModificationsCount = entity.keyModificationsCount,
+ keyConflictsCount = entity.keyConflictsCount,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/branching/BranchMergeModelAssembler.kt
around lines 17-20, the assembler is downcasting merge counter fields with
toInt(), which can overflow; stop converting counts to Int and preserve them as
Longs, update BranchMergeModel (and its constructor/fields) to use Long for
keyAdditionsCount, keyDeletionsCount, keyModificationsCount, keyConflictsCount,
and adjust any call sites or tests that expect Ints so the assembler maps the
entity's Long values directly without toInt() conversions.
ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt
Outdated
Show resolved
Hide resolved
| override fun getBranch(projectId: Long, branchId: Long): Branch { | ||
| return branchRepository.findByProjectIdAndId(projectId, branchId) | ||
| ?: throw BadRequestException(Message.BRANCH_NOT_FOUND) | ||
| } |
There was a problem hiding this comment.
Use NotFoundException instead of BadRequestException.
A missing branch is a resource-not-found scenario (HTTP 404), not a bad request (HTTP 400).
Apply this diff:
override fun getBranch(projectId: Long, branchId: Long): Branch {
return branchRepository.findByProjectIdAndId(projectId, branchId)
- ?: throw BadRequestException(Message.BRANCH_NOT_FOUND)
+ ?: throw NotFoundException(Message.BRANCH_NOT_FOUND)
}🤖 Prompt for AI Agents
In
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt
around lines 54 to 57, the method throws BadRequestException when a branch is
not found; change this to throw NotFoundException instead to represent a 404
resource-not-found. Replace BadRequestException with NotFoundException in the
throw site (keeping Message.BRANCH_NOT_FOUND), and add or update the import for
NotFoundException if necessary.
| @Transactional | ||
| override fun getBranchMergeConflicts( | ||
| projectId: Long, | ||
| branchId: Long, | ||
| branchMergeId: Long, | ||
| pageable: Pageable | ||
| ): Page<BranchMergeConflictView> { | ||
| val project = entityManager.getReference(Project::class.java, projectId) | ||
| val conflicts = branchMergeService.getConflicts(projectId, branchId, branchMergeId, pageable) | ||
| val languages = | ||
| languageService.getLanguagesForTranslationsView( | ||
| project.languages.map { it.tag }.toSet(), | ||
| project.id, | ||
| authenticationFacade.authenticatedUser.id, | ||
| ) | ||
|
|
||
| val sourceFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.sourceBranchKeyId }.toList() } | ||
| val targetFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.targetBranchKeyId }.toList() } | ||
|
|
||
| val sourceKeys = translationViewDataProvider | ||
| .getData(projectId, languages, pageable, sourceFilter) | ||
| .associateBy { it.keyId } | ||
|
|
||
| val targetKeys = translationViewDataProvider | ||
| .getData(projectId, languages, pageable, targetFilter) | ||
| .associateBy { it.keyId } | ||
|
|
||
| conflicts.forEach { conflict -> | ||
| conflict.sourceBranchKey = sourceKeys[conflict.sourceBranchKeyId]!! | ||
| conflict.targetBranchKey = targetKeys[conflict.targetBranchKeyId]!! | ||
| } | ||
| return conflicts | ||
| } |
There was a problem hiding this comment.
Mark as read-only transaction; handle missing keys safely.
- This is a read-only operation—add
@Transactional(readOnly = true)for optimization. - Lines 143-144 use
!!which will throw NPE if a key is missing from the map. Use safe access or validate that all keys are present.
Apply this diff:
- @Transactional
+ @Transactional(readOnly = true)
override fun getBranchMergeConflicts(
projectId: Long,
branchId: Long,
branchMergeId: Long,
pageable: Pageable
): Page<BranchMergeConflictView> {
val project = entityManager.getReference(Project::class.java, projectId)
val conflicts = branchMergeService.getConflicts(projectId, branchId, branchMergeId, pageable)
val languages =
languageService.getLanguagesForTranslationsView(
project.languages.map { it.tag }.toSet(),
project.id,
authenticationFacade.authenticatedUser.id,
)
val sourceFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.sourceBranchKeyId }.toList() }
val targetFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.targetBranchKeyId }.toList() }
val sourceKeys = translationViewDataProvider
.getData(projectId, languages, pageable, sourceFilter)
.associateBy { it.keyId }
val targetKeys = translationViewDataProvider
.getData(projectId, languages, pageable, targetFilter)
.associateBy { it.keyId }
conflicts.forEach { conflict ->
- conflict.sourceBranchKey = sourceKeys[conflict.sourceBranchKeyId]!!
- conflict.targetBranchKey = targetKeys[conflict.targetBranchKeyId]!!
+ conflict.sourceBranchKey = sourceKeys[conflict.sourceBranchKeyId]
+ ?: error("Source key ${conflict.sourceBranchKeyId} not found")
+ conflict.targetBranchKey = targetKeys[conflict.targetBranchKeyId]
+ ?: error("Target key ${conflict.targetBranchKeyId} not found")
}
return conflicts
}🤖 Prompt for AI Agents
In
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt
around lines 115-147, this method is a read-only operation and currently uses
non-null assertions (!!) that can NPE if a key is missing; change the annotation
to @Transactional(readOnly = true) and replace the forced !! lookups with safe
access—either validate that all expected keyIds from conflicts exist in
sourceKeys/targetKeys and throw a clear exception if not, or assign using safe
get (?.) and handle nulls on the conflict (e.g., set to null or a fallback), so
missing keys are handled explicitly rather than causing an NPE.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (1)
467-472: Past review comment not addressed: FK/cascade issue persists.The code still saves
codeReferencesbeforeKeyMetaentities (lines 470-471), which can break referential integrity. The import path (lines 477-478) correctly saves metas first, then code references viakeyMetaService.saveAllCodeReferences.As noted in the previous review, apply this diff to fix the ordering:
private fun saveAllKeyDependants(keyBuilders: List<KeyBuilder>) { val metas = keyBuilders.map { it.data.meta?.self }.filterNotNull() tagService.saveAll(metas.flatMap { it.tags }) - keyCodeReferenceRepository.saveAll(metas.flatMap { it.codeReferences }) - keyMetaService.saveAll(metas) + keyMetaService.saveAll(metas) + keyMetaService.saveAllCodeReferences(metas.flatMap { it.codeReferences }) }After this change, the
keyCodeReferenceRepositorydependency can be removed from the constructor (line 75) and the import (line 8).ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (1)
55-58: Use NotFoundException instead of BadRequestException.A missing branch is a resource-not-found scenario (HTTP 404), not a bad request (HTTP 400).
Apply this diff:
override fun getBranch(projectId: Long, branchId: Long): Branch { return branchRepository.findByProjectIdAndId(projectId, branchId) - ?: throw BadRequestException(Message.BRANCH_NOT_FOUND) + ?: throw NotFoundException(Message.BRANCH_NOT_FOUND) }backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
13-14: Guard the OSS stub bean to avoid EE collisions.This OSS stub still registers unconditionally, so EE builds will see two
BranchServicebeans and fail to start. Add@ConditionalOnMissingBean(BranchService::class)(with the import) to let the real EE implementation take precedence.-import org.springframework.stereotype.Service +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean +import org.springframework.stereotype.Service @@ -@Service +@Service +@ConditionalOnMissingBean(BranchService::class) class BranchServiceOssStub : BranchService {
🧹 Nitpick comments (4)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (1)
600-616: Consider extracting common persistence pattern (optional).All three methods follow an identical pattern: filter entities with
id == 0L, then persist. While the current approach is clear and explicit, you could reduce duplication with a helper function:private fun <T : Any> persistNewEntities(entities: List<T>) { entities.filter { // Assuming entities have an id property via reflection or a common interface (it as? EntityWithId)?.id == 0L }.forEach { entityManager.persist(it) } }However, since this is test code where explicitness can aid readability, the current implementation is acceptable.
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (1)
73-85: Avoid parameter shadowing and return 201 Created for resource creation.The parameter
branchis shadowed by the localval branch, which reduces readability. Additionally, POST endpoints that create resources should return HTTP 201 (Created) rather than the default 200 (OK).Apply this diff:
+import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ResponseStatus @PostMapping(value = [""]) @Operation(summary = "Create branch") @AllowApiAccess @RequiresProjectPermissions([Scope.KEYS_EDIT]) @OpenApiOrderExtension(2) - fun create(@RequestBody branch: CreateBranchModel): BranchModel { - val branch = branchService.createBranch( + @ResponseStatus(HttpStatus.CREATED) + fun create(@RequestBody request: CreateBranchModel): BranchModel { + val branch = branchService.createBranch( projectHolder.project.id, - branch.name, - branch.originBranchId + request.name, + request.originBranchId ) return branchModelAssembler.toModel(branch) }ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
60-82: Remove redundant project validation.Line 62 already retrieves the branch filtered by
projectId, so the branch is guaranteed to belong to the project. The additional check at lines 66-68 is redundant.Apply this diff:
@Transactional override fun createBranch(projectId: Long, name: String, originBranchId: Long): Branch { val originBranch = branchRepository.findByProjectIdAndId(projectId, originBranchId) ?: throw BadRequestException( Message.ORIGIN_BRANCH_NOT_FOUND ) - if (originBranch.project.id != projectId) { - throw BadRequestException(Message.ORIGIN_BRANCH_NOT_FOUND) - } - val branch = createBranch(projectId, name).also { it.originBranch = originBranch it.pending = true }
116-147: Mark as read-only transaction and avoid unsafe null assertions.This method performs only read operations and should be marked
readOnly = truefor optimization. Lines 143-144 use the!!operator, which will throw NPE if keys are missing from the maps—this could happen if translation data is inconsistent or deleted concurrently.Apply this diff:
- @Transactional + @Transactional(readOnly = true) override fun getBranchMergeConflicts( projectId: Long, branchMergeId: Long, pageable: Pageable ): Page<BranchMergeConflictView> { val project = entityManager.getReference(Project::class.java, projectId) val conflicts = branchMergeService.getConflicts(projectId, branchMergeId, pageable) val languages = languageService.getLanguagesForTranslationsView( project.languages.map { it.tag }.toSet(), project.id, authenticationFacade.authenticatedUser.id, ) val sourceFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.sourceBranchKeyId }.toList() } val targetFilter = TranslationFilters().apply { filterKeyId = conflicts.map { it.targetBranchKeyId }.toList() } val sourceKeys = translationViewDataProvider .getData(projectId, languages, pageable, sourceFilter) .associateBy { it.keyId } val targetKeys = translationViewDataProvider .getData(projectId, languages, pageable, targetFilter) .associateBy { it.keyId } conflicts.forEach { conflict -> - conflict.sourceBranchKey = sourceKeys[conflict.sourceBranchKeyId]!! - conflict.targetBranchKey = targetKeys[conflict.targetBranchKeyId]!! + conflict.sourceBranchKey = sourceKeys[conflict.sourceBranchKeyId] + ?: error("Source key ${conflict.sourceBranchKeyId} not found") + conflict.targetBranchKey = targetKeys[conflict.targetBranchKeyId] + ?: error("Target key ${conflict.targetBranchKeyId} not found") } return conflicts }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt(5 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeBuilder.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeChangeBuilder.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/DryRunMergeBranchRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/ResolveBranchMergeConflictRequest.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeChangeRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-11T14:43:26.099Z
Learnt from: stepan662
Repo: tolgee/tolgee-platform PR: 3124
File: backend/data/src/main/kotlin/io/tolgee/batch/MtProviderCatching.kt:38-39
Timestamp: 2025-06-11T14:43:26.099Z
Learning: In Tolgee Platform, backend Message enum keys are not looked up via `messages*.properties`; localization is performed in the frontend, so adding new enum constants does not require backend properties entries.
Applied to files:
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt
🧬 Code graph analysis (2)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (2)
createBranch(15-15)getBranch(14-14)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (2)
createBranch(23-25)getBranch(19-21)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addTranslation(146-146)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
addMeta(44-47)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeBuilder.kt (1)
addChange(12-14)
🔇 Additional comments (13)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt (2)
8-8: LGTM: Clean dependency injection.The KeyCodeReferenceRepository dependency is properly added to the constructor and will be used for persisting code references.
Also applies to: 75-75
289-292: No issues found. The persistence order correctly respects FK dependencies:
saveBranchMergeChangesdepends on bothBranchMerge(non-nullable) andKeyentities (nullable viasourceKeyandtargetKey)- All dependencies are satisfied by the current order:
saveBranches→saveBranchMerges→saveKeyData→saveBranchMergeChangesbackend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeChangeBuilder.kt (1)
1-12: LGTM! Clean builder implementation.The builder follows established patterns in the codebase. The optional
branchMergeBuilderparameter enables flexible construction while properly establishing the relationship when provided.backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (3)
8-10: LGTM! Proper imports added.The imports for the new branching entities are correctly added.
71-73: LGTM! Collections added consistently.The three new collections for branches, branch merges, and branch merge changes follow the existing pattern in the DATA class.
94-106: LGTM! Well-designed mutator methods.The mutator methods follow the established pattern in ProjectBuilder. The overloaded
addBranchMergeChangemethod (lines 100-106) properly establishes the relationship between BranchMerge and BranchMergeChange by creating a BranchMergeChangeBuilder with the provided BranchMergeBuilder, enabling fluent construction of related entities in test data.backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeBuilder.kt (1)
1-15: LGTM! Clean builder with proper delegation.The builder follows established patterns. The
addChangemethod correctly delegates to ProjectBuilder's overloaded method, passingthisto establish the bidirectional relationship between BranchMerge and BranchMergeChange.backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/ResolveBranchMergeConflictRequest.kt (1)
1-12: LGTM!Clean DTO structure with appropriate Schema annotations for API documentation.
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt (1)
312-317: LGTM!New branch-related message constants follow the established pattern and are correctly positioned. Based on learnings, these don't require backend properties entries since localization is handled in the frontend.
backend/data/src/main/kotlin/io/tolgee/dtos/request/branching/DryRunMergeBranchRequest.kt (1)
1-11: LGTM!Well-structured DTO with clear property names and appropriate documentation.
ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt (1)
10-30: LGTM!The JPQL query correctly scopes by project using
sb.project.id = :projectIdand properly aggregates change counts by type.ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeChangeRepository.kt (1)
33-43: LGTM!The
findConflictmethod correctly scopes by project through the source branch relationship.backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (1)
12-26: LGTM!Clean interface contract with well-defined method signatures for branch management and merge workflows.
| @Query( | ||
| """ | ||
| select new io.tolgee.dtos.queryResults.branching.BranchMergeConflictView( | ||
| bmc.id, | ||
| bmc.sourceKey.id, | ||
| bmc.targetKey.id, | ||
| bmc.resolution | ||
| ) | ||
| from BranchMergeChange bmc | ||
| join bmc.branchMerge bm | ||
| WHERE | ||
| bm.id = :mergeId | ||
| and bmc.change = io.tolgee.model.enums.BranchKeyMergeChangeType.CONFLICT | ||
| """ | ||
| ) | ||
| fun findBranchMergeConflicts( | ||
| projectId: Long, | ||
| mergeId: Long, | ||
| pageable: Pageable | ||
| ): Page<BranchMergeConflictView> |
There was a problem hiding this comment.
Critical: Unused projectId parameter causes runtime failure and enables cross-project data access.
The findBranchMergeConflicts method declares a projectId parameter but never references it in the JPQL query. This will cause Spring Data to throw IllegalArgumentException: Named parameter [projectId] not bound at runtime. Even if the binding issue were overlooked, the absence of project scoping would allow unauthorized access to merge conflicts from other projects if someone knew valid merge IDs.
Apply this diff to bind the parameter and scope by project:
@Query(
"""
select new io.tolgee.dtos.queryResults.branching.BranchMergeConflictView(
bmc.id,
bmc.sourceKey.id,
bmc.targetKey.id,
bmc.resolution
)
from BranchMergeChange bmc
join bmc.branchMerge bm
+ join bm.sourceBranch sb
WHERE
+ sb.project.id = :projectId
+ and bm.id = :mergeId
- bm.id = :mergeId
and bmc.change = io.tolgee.model.enums.BranchKeyMergeChangeType.CONFLICT
"""
)📝 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.
| @Query( | |
| """ | |
| select new io.tolgee.dtos.queryResults.branching.BranchMergeConflictView( | |
| bmc.id, | |
| bmc.sourceKey.id, | |
| bmc.targetKey.id, | |
| bmc.resolution | |
| ) | |
| from BranchMergeChange bmc | |
| join bmc.branchMerge bm | |
| WHERE | |
| bm.id = :mergeId | |
| and bmc.change = io.tolgee.model.enums.BranchKeyMergeChangeType.CONFLICT | |
| """ | |
| ) | |
| fun findBranchMergeConflicts( | |
| projectId: Long, | |
| mergeId: Long, | |
| pageable: Pageable | |
| ): Page<BranchMergeConflictView> | |
| @Query( | |
| """ | |
| select new io.tolgee.dtos.queryResults.branching.BranchMergeConflictView( | |
| bmc.id, | |
| bmc.sourceKey.id, | |
| bmc.targetKey.id, | |
| bmc.resolution | |
| ) | |
| from BranchMergeChange bmc | |
| join bmc.branchMerge bm | |
| join bm.sourceBranch sb | |
| WHERE | |
| sb.project.id = :projectId | |
| and bm.id = :mergeId | |
| and bmc.change = io.tolgee.model.enums.BranchKeyMergeChangeType.CONFLICT | |
| """ | |
| ) | |
| fun findBranchMergeConflicts( | |
| projectId: Long, | |
| mergeId: Long, | |
| pageable: Pageable | |
| ): Page<BranchMergeConflictView> |
...kend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt (1)
78-84: Fix parameter shadowing and return 201 Created.The parameter
branchis shadowed by the local variable on line 79, reducing readability. Additionally, resource creation endpoints should return HTTP 201 Created rather than 200 OK.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
55-58: Use NotFoundException for missing resource.A missing branch represents a resource-not-found scenario (HTTP 404), not a bad request (HTTP 400). This was previously flagged and remains unresolved.
115-146: Mark as read-only transaction and handle missing keys safely.Two issues remain from previous review:
- This read-only operation should be annotated
@Transactional(readOnly = true)for optimization.- Lines 142-143 use
!!which will throw NPE if keys are missing from the map.backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
111-112: NPE risk already flagged in previous review.This issue with
updatedAt!!has already been identified in a previous review comment. Please address the earlier feedback to use a safe fallback.backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt (4)
85-85: Fix indexed and order-dependentcommentsaccesses now that it's a Set.This issue was already raised in a previous review. The change from
MutableListtoMutableSetbreaks code that relies on indexed access or ordering. Test files need to be updated accordingly.
206-208: isModified() likely always returns true due to improper null handling.This issue was already raised in a previous review. The method doesn't handle missing keys in
oldStateproperly and should accept nullable values. The comparison can spuriously return true when keys are missing.
210-212: differsInBranchVersion() stub always returns true.This issue was already raised in a previous review. The stub implementation incorrectly reports that all translations differ across branches. A real field-by-field comparison of text, state, and labels is needed.
214-219: Inverse side consistency concern remains.This issue was partially addressed from a previous review. The code now correctly uses
clear()andaddAll()instead of reassigning the collection, which is an improvement. However, the previous review's concern about usingaddLabel/removeLabelhelper methods to maintain explicit inverse side consistency withLabel.translationsstill applies.While JPA's dirty tracking should handle the join table for managed entities, using the helper methods would ensure bidirectional consistency even when entities are cached in memory within the same session.
🧹 Nitpick comments (4)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
19-53: Add informative error messages to UnsupportedOperationException.The stub methods correctly throw
UnsupportedOperationException, but adding a descriptive message would improve the developer experience by clarifying that these features are EE-only.Apply this diff to add informative error messages:
override fun getBranch(projectId: Long, branchId: Long): Branch { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun createBranch(projectId: Long, name: String, originBranchId: Long): Branch { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun deleteBranch(projectId: Long, branchId: Long) { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun dryRunMergeBranch(projectId: Long, request: DryRunMergeBranchRequest): BranchMerge { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun getBranchMergeView(projectId: Long, mergeId: Long): BranchMergeView { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun getBranchMergeConflicts( projectId: Long, branchMergeId: Long, pageable: Pageable ): Page<BranchMergeConflictView> { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun resolveConflict(projectId: Long, mergeId: Long, request: ResolveBranchMergeConflictRequest) { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") } override fun applyMerge(projectId: Long, mergeId: Long) { - throw UnsupportedOperationException() + throw UnsupportedOperationException("Branching is available in EE only") }backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt (1)
50-51: Consider more idiomatic Kotlin:all { it.isResolved }.The expression
changes.none { !it.isResolved }is logically correct but less readable thanchanges.all { it.isResolved }.Apply this diff:
val isResolved: Boolean - get() = changes.none { !it.isResolved } + get() = changes.all { it.isResolved }ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt (2)
73-82: Simplify: remove redundantapplyblock.The
applyblock on lines 78-81 is unnecessary sincechangeis already in scope. You can assign properties directly.Apply this diff:
sourceOnly.forEach { sourceKey -> val change = BranchMergeChange().apply { this.branchMerge = merge this.sourceKey = sourceKey - } - change.apply { - change.change = BranchKeyMergeChangeType.ADD - change.resolution = BranchKeyMergeResolutionType.SOURCE + this.change = BranchKeyMergeChangeType.ADD + this.resolution = BranchKeyMergeResolutionType.SOURCE } resolutions.add(change) }
86-95: Simplify: remove redundantapplyblock.The
applyblock on lines 91-94 is unnecessary sincechangeis already in scope. You can assign properties directly.Apply this diff:
targetOnly.forEach { targetKey -> val change = BranchMergeChange().apply { this.branchMerge = merge this.targetKey = targetKey - } - change.apply { - change.change = BranchKeyMergeChangeType.DELETE - change.resolution = BranchKeyMergeResolutionType.TARGET + this.change = BranchKeyMergeChangeType.DELETE + this.resolution = BranchKeyMergeResolutionType.TARGET } resolutions.add(change) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMerge.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMergeChange.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt(9 hunks)backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt(4 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchController.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchMergeService.kt(1 hunks)ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/BranchTestData.kt
- backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt
- backend/data/src/main/kotlin/io/tolgee/model/branching/BranchMergeChange.kt
- ee/backend/app/src/main/kotlin/io/tolgee/ee/repository/branching/BranchMergeRepository.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-11T14:43:26.099Z
Learnt from: stepan662
Repo: tolgee/tolgee-platform PR: 3124
File: backend/data/src/main/kotlin/io/tolgee/batch/MtProviderCatching.kt:38-39
Timestamp: 2025-06-11T14:43:26.099Z
Learning: In Tolgee Platform, backend Message enum keys are not looked up via `messages*.properties`; localization is performed in the frontend, so adding new enum constants does not require backend properties entries.
Applied to files:
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt
🧬 Code graph analysis (2)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerTest.kt (3)
backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/ProjectBuilder.kt (1)
addTranslation(146-146)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/KeyBuilder.kt (1)
addMeta(44-47)backend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/builders/BranchMergeBuilder.kt (1)
addChange(12-14)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchServiceImpl.kt (2)
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchService.kt (2)
createBranch(15-15)getBranch(14-14)backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (2)
createBranch(23-25)getBranch(19-21)
🔇 Additional comments (4)
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt (1)
312-319: LGTM! Remember to add frontend localizations.The eight new branch-related message constants are well-named, follow the existing naming conventions, and appropriately cover key branching scenarios (not found errors, delete restrictions, merge operations, and conflict resolution). The placement before the terminating semicolon is correct.
Based on learnings, ensure these message codes are localized in the frontend, as backend
messages*.propertiesentries are not required for the Message enum.backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt (1)
15-17: LGTM!Returning an empty page for the list operation is an appropriate OSS stub behavior.
backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
71-75: LGTM! Well-designed backward compatibility.The nullable branch field with a clear comment about backward compatibility is a good design choice. The lazy fetching is also appropriate for performance.
backend/data/src/main/kotlin/io/tolgee/model/translation/Translation.kt (1)
204-204: LGTM!The
resolveKeyId()implementation correctly returns the key ID for branch versioning support.
| override fun differsInBranchVersion(entity: Key): Boolean { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Implement actual comparison logic.
The method always returns true without comparing any properties between this and the entity parameter. This will cause incorrect behavior in branch versioning logic.
Apply this diff to implement proper comparison:
override fun differsInBranchVersion(entity: Key): Boolean {
- return true
+ return this.name != entity.name ||
+ this.isPlural != entity.isPlural ||
+ this.pluralArgName != entity.pluralArgName
}Note: You may also need to compare translations and keyMeta depending on the branching requirements.
📝 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.
| override fun differsInBranchVersion(entity: Key): Boolean { | |
| return true | |
| } | |
| override fun differsInBranchVersion(entity: Key): Boolean { | |
| return this.name != entity.name || | |
| this.isPlural != entity.isPlural || | |
| this.pluralArgName != entity.pluralArgName | |
| } |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt around lines 146 to
148, the differsInBranchVersion method currently always returns true; replace it
with a real comparison that checks relevant fields between this and the provided
entity (at minimum compare name and namespace, and depending on branching rules
also compare type, keyMeta, and the translations collection). Implement
null-safe equality checks for primitive/nullable fields, and for collections
compare size and element-wise equality (or use equals/hashCode on
translation/keyMeta entities) so the method only returns true when differences
exist.
| override fun merge(source: Key) { | ||
| this.isPlural = source.isPlural | ||
| this.pluralArgName = source.pluralArgName | ||
| this.translations.forEach { translation -> | ||
| translation.merge(source.translations.find { it.language == translation.language }!!) | ||
| } | ||
| this.keyMeta?.let { meta -> | ||
| source.keyMeta?.let { sourceMeta -> | ||
| meta.merge(sourceMeta) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: NPE risk in translation merge logic.
Line 154 uses a force unwrap (!!) when finding matching translations, which will throw an NPE if a translation in this.translations doesn't have a corresponding translation in source.translations with the same language.
Apply this diff to handle missing translations safely:
override fun merge(source: Key) {
this.isPlural = source.isPlural
this.pluralArgName = source.pluralArgName
this.translations.forEach { translation ->
- translation.merge(source.translations.find { it.language == translation.language }!!)
+ source.translations.find { it.language == translation.language }?.let { sourceTranslation ->
+ translation.merge(sourceTranslation)
+ }
}
this.keyMeta?.let { meta ->
source.keyMeta?.let { sourceMeta ->
meta.merge(sourceMeta)
}
}
}Additionally, consider whether translations present in source but not in this should be added to this.translations.
| @Service | ||
| class BranchServiceOssStub : BranchService { |
There was a problem hiding this comment.
Missing @ConditionalOnMissingBean annotation causes bean collision in EE.
Without this annotation, Spring will register both the OSS stub and the EE implementation, causing a duplicate bean error at startup in EE builds.
Apply this diff to fix the bean collision:
import io.tolgee.model.branching.Branch
import io.tolgee.model.branching.BranchMerge
import org.springframework.data.domain.Page
import org.springframework.data.domain.Pageable
import org.springframework.stereotype.Service
+import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
@Service
+@ConditionalOnMissingBean(BranchService::class)
class BranchServiceOssStub : BranchService {📝 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.
| @Service | |
| class BranchServiceOssStub : BranchService { | |
| import io.tolgee.model.branching.Branch | |
| import io.tolgee.model.branching.BranchMerge | |
| import org.springframework.data.domain.Page | |
| import org.springframework.data.domain.Pageable | |
| import org.springframework.stereotype.Service | |
| import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean | |
| @Service | |
| @ConditionalOnMissingBean(BranchService::class) | |
| class BranchServiceOssStub : BranchService { |
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/service/branching/BranchServiceOssStub.kt
around lines 13 to 14, the OSS stub is missing the @ConditionalOnMissingBean
annotation which causes bean collision with the EE implementation; add
@ConditionalOnMissingBean(BranchService::class) (importing
org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean) above
the class declaration so the OSS stub is only registered when no other
BranchService bean exists.
… handle branch in services
…load (branch name in url)
…on-deleted branch
dkrizan
force-pushed
the
danielkrizan/branching
branch
from
6b3a8bd to
e56806b
Compare
Summary by CodeRabbit