feat: branching improvements - #3474
Merged
Merged
Conversation
Replace JPA-based createInitialSnapshot() with pure SQL operations, eliminating cartesian product queries, full heap loading, and per-row ORM inserts. Also fix Key.hasChanged() to deduplicate translations before comparing, which was masked by the old code producing matching duplicates on both sides. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved translation comparison to deduplicate by language for more reliable change detection. * Reworked branch snapshot creation to use database-side mapping and inserts, reducing in-memory processing and improving snapshot performance and consistency. * Removed an obsolete repository fetch method and updated snapshot service construction to no longer require that dependency. * Ensured merge flow flushes pending changes before rebuilding snapshots. * **Tests** * Adjusted transactional handling in branching tests to match the new snapshot flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Replace the broken direct-join query in `BranchMergeActivityParamsProvider` with projection queries that resolve source/target branch names via activity describing relations and entity data - Add `@ActivityEntityDescribingPaths(["sourceBranch", "targetBranch"])` to `BranchMerge` so the activity system records branch references - Add integration test verifying merge activity params contain correct branch names ## Test plan - [x] `BranchControllerMergingTest` — new test `merge activity includes source and target branch names in params` passes - [x] `ProjectActivityBranchingTest` — existing tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added test coverage for branch merge activity logging to verify source and target branch information is properly captured. * **Refactor** * Improved internal handling of branch merge activity data collection to ensure complete information is recorded in activity logs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Links CDN configs to branches via a foreign key (`branch_id`) instead of the plain `filterBranch` string column - Adds branch selector to the CDN config dialog when branching is enabled - Shows branch name chip on CDN list items - Cleans up CDN configs when a branch is deleted - Migrates existing configs to the default branch during branching enablement - Feature guard rejects CDN create/update with `filterBranch` when BRANCHING feature is disabled ## Test plan - [x] All 12 `ContentDeliveryConfigBranchingTest` tests pass, including: - Create/update CDN config with specific branch - Create without branch defaults to default branch - Non-existent branch returns 404 - Delete branch cascades to CDN configs - Feature guard rejects create when branching not enabled - Feature guard rejects update when branching not enabled 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…merge deletion functionality
Contributor
📝 WalkthroughWalkthroughAdds branch-awareness to Content Delivery configs: replaces string filterBranch with a branch relation/branchName across models, repos, services, DB migration, UI, tests, and branches the CDN publish flow to only publish when the activity modified entities belong to the config's branch. Changes
Sequence Diagram(s)sequenceDiagram
participant Automation as Automation Trigger
participant Processor as ContentDeliveryPublishProcessor
participant Activity as ActivityService/Repo
participant Uploader as ContentDeliveryUploader
participant Storage as Storage Provider
Automation->>Processor: on translation revision (activityRevisionId)
Processor->>Activity: hasModifiedEntitiesOnBranch(revisionId, config.branchId)?
alt modified on config branch
Processor->>Uploader: upload(config, revision)
Uploader->>Storage: storeFile(...)
Storage-->>Uploader: success/ok
Uploader-->>Processor: uploaded
else not modified on config branch
Processor-->>Automation: return (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dkrizan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt (1)
223-224:⚠️ Potential issue | 🟡 MinorDuplicate assertion — likely copy-paste error.
refreshedBranch.deletedAt.assert.isNull()is asserted on both Line 223 and Line 224. The second assertion appears to be a leftover; perhaps it was intended to check a different property (e.g., that the snapshot was reset).Proposed fix
val refreshedBranch = testData.featureBranch.refresh()!! refreshedBranch.deletedAt.assert.isNull() - refreshedBranch.deletedAt.assert.isNull()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt` around lines 223 - 224, There is a duplicated assertion of refreshedBranch.deletedAt; remove the second duplicated line and replace it with an assertion that the branch snapshot was reset — e.g. assert that refreshedBranch.snapshotId (or refreshedBranch.snapshot) is null by adding refreshedBranch.snapshotId.assert.isNull() (or refreshedBranch.snapshot.assert.isNull()) to verify the snapshot was cleared.
🧹 Nitpick comments (9)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt (1)
642-642: Parameterkeyis shadowed by localval key.The local variable
val keyshadows the function parameter of the same name. While this works, it can be confusing during maintenance.Proposed fix
- val key = keyService.getKeysWithTagsById(key.project.id, listOf(key.id)).singleOrNull() ?: throw NotFoundException() - tagService.tagKey(key, tagName) + val keyWithTags = keyService.getKeysWithTagsById(key.project.id, listOf(key.id)).singleOrNull() ?: throw NotFoundException() + tagService.tagKey(keyWithTags, tagName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt` at line 642, A local variable named `key` shadows the function parameter `key`; rename the local `val key = keyService.getKeysWithTagsById(...)` to a non-conflicting name (e.g., `keyWithTags` or `keyDto`) so references to the parameter and the fetched object are unambiguous; update subsequent usages in this test (the result of `getKeysWithTagsById`) to use the new identifier and leave the method parameter `key` unchanged.webapp/src/views/projects/developer/contentDelivery/CdDialog.tsx (1)
42-42: UnusedReactimport.There are no direct
React.*references in this file, and with the React 17+ JSX transform, an explicitReactimport is not needed for JSX.Remove unused import
-import React from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/views/projects/developer/contentDelivery/CdDialog.tsx` at line 42, The file-level import "import React from 'react';" is unused in the CdDialog component; remove that import statement from CdDialog.tsx (or replace it with only the necessary type imports if you need types) so the module no longer contains an unused React import.ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt (1)
132-168: Test looks correct; consider extracting the shared setup.The setup logic (Lines 135-152) is nearly identical to
merges resolved feature branch into main(Lines 101-119). This is fine for now given it's a test file, but if more merge-activity tests are added, extracting a helper likeapplyMergeWithConflicts(): Longwould reduce duplication.The polling assertion approach using
waitForNotThrowingis appropriate for async activity processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt` around lines 132 - 168, Extract the repeated setup in the test method `merge activity includes source and target branch names in params` (the block using `initConflicts()`, the `waitForNotThrowing` refresh/asserts, the `createMergeWithConflict(...)` call, and the `performProjectAuthPost("branches/merge/$mergeId/apply")`) into a helper like `applyMergeWithConflicts(): Long` that returns the `mergeId`; update this test and the other similar test (`merges resolved feature branch into main`) to call that helper instead of duplicating the same lines, keeping the existing polling/validation logic in the tests themselves.backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt (1)
166-177:distinctBydeduplication looks correct but uses O(n²) lookup — considerassociateByfor consistency.The deduplication fix is sound. However, the
findon Line 173 is O(n) per iteration, making this loop O(n²). TheisConflictingmethod just below (Lines 206-208) usesassociateByfor O(1) lookups on the same kind of data. For consistency and marginal performance improvement:♻️ Suggested refactor using associateBy
- val distinctTranslations = this.translations.distinctBy { it.language.tag } - val distinctSnapshotTranslations = snapshot.translations.distinctBy { it.language } - if (distinctTranslations.size != distinctSnapshotTranslations.size) { + val distinctTranslations = this.translations.associateBy { it.language.tag } + val distinctSnapshotTranslations = snapshot.translations.associateBy { it.language } + if (distinctTranslations.size != distinctSnapshotTranslations.size) { return true } - for (translation in distinctTranslations) { - val snapshotTranslation = - distinctSnapshotTranslations.find { it.language == translation.language.tag } ?: return true + for ((langTag, translation) in distinctTranslations) { + val snapshotTranslation = distinctSnapshotTranslations[langTag] ?: return true if (translation.hasChanged(snapshotTranslation)) { return true } }Note:
associateByalso keeps the last element per key (vsdistinctBywhich keeps the first). If duplicate translations per language are possible, verify which behavior is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt` around lines 166 - 177, The current loop in Key.kt builds distinctTranslations and distinctSnapshotTranslations as lists and uses find inside the loop, causing O(n²) behavior; change distinctSnapshotTranslations to a Map by using associateBy (e.g., snapshot.translations.associateBy { it.language } or { it.language.tag } to match keys used) so you can do O(1) lookups when checking each translation in the loop (refer to distinctTranslations, distinctSnapshotTranslations, hasChanged and isConflicting for where to apply the change); ensure you choose the correct key (language vs language.tag) and be aware that associateBy keeps the last element for duplicate keys versus distinctBy keeping the first.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchSnapshotService.kt (1)
32-42: Consider documenting the default-branch NULL convention.The
getBranchFilterhelper encodes the assumption that default-branch keys may havebranch_id IS NULL. A brief KDoc comment on this method explaining the convention would help future maintainers understand why theOR … IS NULLclause is needed for default branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchSnapshotService.kt` around lines 32 - 42, Add a KDoc above the getBranchFilter function explaining the default-branch convention: state that keys belonging to the default branch are stored with branch_id == NULL and therefore when isDefault is true the SQL must match either the given branch_id parameter or NULL; mention the expected semantics and any callers that rely on this behavior (e.g., callers that pass isDefault=true to include default-branch keys). Ensure the KDoc is concise and placed immediately above the getBranchFilter function.backend/app/src/test/kotlin/io/tolgee/automation/ContentDeliveryBranchAutopushTest.kt (1)
49-67: Avoid fixed sleep; poll for initial invocations instead.
Thread.sleep(1000)can be flaky on slower/faster machines. Consider waiting via a polling helper (e.g.,waitForNotThrowing) until the expected initial invocations appear, then clear them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/test/kotlin/io/tolgee/automation/ContentDeliveryBranchAutopushTest.kt` around lines 49 - 67, The test's setup() uses Thread.sleep(1000) to wait for initial invocations which is flaky; replace this with a polling wait that checks for the expected invocation(s) on fileStorageMock and only then calls Mockito.clearInvocations(fileStorageMock). Specifically, remove Thread.sleep(1000) and use an existing polling helper (e.g., waitForNotThrowing) to repeatedly verify the initial invocation(s) on fileStorageMock (or another observable call caused by testDataService.saveTestData in setup()) until the verification succeeds, then invoke Mockito.clearInvocations(fileStorageMock) to reset interactions.backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt (1)
227-239: Consider reusing the automation-cleanup logic fromdelete().Lines 233–237 duplicate the automation-deletion pattern from
delete()(lines 203–206). Extracting a small private helper (e.g.,deleteConfigWithAutomations(config)) would keep them in sync if the teardown logic evolves.♻️ Suggested refactor
+ private fun deleteConfigWithAutomations(config: ContentDeliveryConfig) { + config.automationActions.map { it.automation }.forEach { + automationService.delete(it) + } + contentDeliveryConfigRepository.deleteById(config.id) + } + fun delete( projectId: Long, id: Long, ) { val config = get(projectId, id) - config.automationActions.map { it.automation }.forEach { - automationService.delete(it) - } - contentDeliveryConfigRepository.deleteById(config.id) + deleteConfigWithAutomations(config) } // ... `@Transactional` fun deleteAllByBranchId( projectId: Long, branchId: Long, ) { val configs = contentDeliveryConfigRepository.findAllByProjectIdAndBranchId(projectId, branchId) configs.forEach { config -> - config.automationActions.map { it.automation }.forEach { - automationService.delete(it) - } - contentDeliveryConfigRepository.deleteById(config.id) + deleteConfigWithAutomations(config) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt` around lines 227 - 239, deleteAllByBranchId duplicates the automation-teardown logic from delete(); extract a private helper (e.g., deleteConfigWithAutomations(config: ContentDeliveryConfig)) that performs config.automationActions.map { it.automation }.forEach { automationService.delete(it) } and contentDeliveryConfigRepository.deleteById(config.id), then call this helper from both delete(projectId, id) and deleteAllByBranchId(projectId, branchId) so the cleanup logic is centralized and stays in sync.backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt (1)
147-150: Minor: Use_for the unused setter parameter for consistency.The model class (
ContentDeliveryConfigModel) usesset(_) {}but here it usesset(value) {}. Using_is idiomatic Kotlin for intentionally unused parameters and signals intent more clearly.Suggested fix
`@get`:Transient override var filterBranch: String? get() = branch?.name - set(value) {} // no-op; branch FK is the source of truth + set(_) {} // no-op; branch FK is the source of truth🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt` around lines 147 - 150, Change the unused setter parameter for the property filterBranch in the ContentDeliveryConfig class from set(value) {} to the idiomatic Kotlin set(_) {} so it clearly signals the parameter is intentionally unused; update the setter on the filterBranch property in ContentDeliveryConfig (or ContentDeliveryConfigModel) accordingly.ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/ContentDeliveryConfigBranchingTest.kt (1)
160-170: Nit: Prefer assertion over!!for better failure messages.Line 166 uses
config.branch!!which would throw an NPE ifbranchis unexpectedly null, producing an opaque stack trace instead of a clear assertion failure.Suggested tweak
executeInNewTransaction { val config = contentDeliveryConfigService.get(testData.featureBranchCdnConfig.self.id) config.filterBranch.assert.isEqualTo("feature") - config.branch!! - .name.assert + config.branch.assert.isNotNull() + config.branch!!.name.assert .isEqualTo("feature") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/ContentDeliveryConfigBranchingTest.kt` around lines 160 - 170, The test uses a hard non-null assertion config.branch!! which can throw an NPE and give poor failure context; change it to assert the branch is present first and then use the asserted value (e.g. call config.branch.assert.isNotNull() then assign val branch = config.branch!! or use config.branch?.let { it.name.assert.isEqualTo("feature") }) so the test fails with a clear assertion message instead of an NPE; update the test method CDN config filterBranch is derived from branch entity (ContentDeliveryConfigBranchingTest) to perform the null-check/assert before accessing branch.name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/data/src/main/resources/db/changelog/schema.xml`:
- Around line 5060-5079: Add a new data-migration changeSet between the
addColumn (id="1770751028716-1")/createIndex (id="1770751028716-2") and the
addForeignKey/dropColumn changeSets to backfill
content_delivery_config.branch_id from the existing filter_branch values: run an
UPDATE on content_delivery_config that sets branch_id = (SELECT id FROM branch b
WHERE b.<unique_identifier_column> = content_delivery_config.filter_branch) (or
use a JOIN on the appropriate branch column), skip NULLs and avoid overwriting
non-null branch_id, and ensure this changeSet runs before the FK creation
(id="1770751028716-3") and before dropping filter_branch (id="1770751028716-4");
optionally add a rollback that repopulates filter_branch from branch_id by
JOINing branch.id to branch.<unique_identifier_column>.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchSnapshotService.kt`:
- Around line 117-120: The SQL in BranchSnapshotService (in
BranchSnapshotService.kt) currently uses coalesce(t.text, '') which turns NULL
translation.text into an empty string; update the SELECT to preserve NULLs
instead of coercing them—remove the coalesce and select t.text directly (or
explicitly coalesce to NULL if needed by the query builder), or alternatively
ensure any comparison logic elsewhere (e.g., snapshot diff code in
BranchSnapshotService) applies the same coalesce on both sides if you
intentionally want to treat NULL and '' as equal; locate the SQL block that
contains "coalesce(t.text, '')" and replace it with "t.text" (or a
NULL-preserving expression).
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt`:
- Around line 53-54: The field activityTestUtil is injected but never used;
remove the unused declaration "lateinit var activityTestUtil: ActivityTestUtil"
from BranchControllerMergingTest and also delete the corresponding import for
ActivityTestUtil to avoid dead code and confusion—verify tests still compile and
that the test at Line 134 (which uses performProjectAuthGet("activity")) remains
unchanged.
---
Outside diff comments:
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt`:
- Around line 223-224: There is a duplicated assertion of
refreshedBranch.deletedAt; remove the second duplicated line and replace it with
an assertion that the branch snapshot was reset — e.g. assert that
refreshedBranch.snapshotId (or refreshedBranch.snapshot) is null by adding
refreshedBranch.snapshotId.assert.isNull() (or
refreshedBranch.snapshot.assert.isNull()) to verify the snapshot was cleared.
---
Nitpick comments:
In
`@backend/app/src/test/kotlin/io/tolgee/automation/ContentDeliveryBranchAutopushTest.kt`:
- Around line 49-67: The test's setup() uses Thread.sleep(1000) to wait for
initial invocations which is flaky; replace this with a polling wait that checks
for the expected invocation(s) on fileStorageMock and only then calls
Mockito.clearInvocations(fileStorageMock). Specifically, remove
Thread.sleep(1000) and use an existing polling helper (e.g., waitForNotThrowing)
to repeatedly verify the initial invocation(s) on fileStorageMock (or another
observable call caused by testDataService.saveTestData in setup()) until the
verification succeeds, then invoke Mockito.clearInvocations(fileStorageMock) to
reset interactions.
In
`@backend/data/src/main/kotlin/io/tolgee/model/contentDelivery/ContentDeliveryConfig.kt`:
- Around line 147-150: Change the unused setter parameter for the property
filterBranch in the ContentDeliveryConfig class from set(value) {} to the
idiomatic Kotlin set(_) {} so it clearly signals the parameter is intentionally
unused; update the setter on the filterBranch property in ContentDeliveryConfig
(or ContentDeliveryConfigModel) accordingly.
In `@backend/data/src/main/kotlin/io/tolgee/model/key/Key.kt`:
- Around line 166-177: The current loop in Key.kt builds distinctTranslations
and distinctSnapshotTranslations as lists and uses find inside the loop, causing
O(n²) behavior; change distinctSnapshotTranslations to a Map by using
associateBy (e.g., snapshot.translations.associateBy { it.language } or {
it.language.tag } to match keys used) so you can do O(1) lookups when checking
each translation in the loop (refer to distinctTranslations,
distinctSnapshotTranslations, hasChanged and isConflicting for where to apply
the change); ensure you choose the correct key (language vs language.tag) and be
aware that associateBy keeps the last element for duplicate keys versus
distinctBy keeping the first.
In
`@backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt`:
- Around line 227-239: deleteAllByBranchId duplicates the automation-teardown
logic from delete(); extract a private helper (e.g.,
deleteConfigWithAutomations(config: ContentDeliveryConfig)) that performs
config.automationActions.map { it.automation }.forEach {
automationService.delete(it) } and
contentDeliveryConfigRepository.deleteById(config.id), then call this helper
from both delete(projectId, id) and deleteAllByBranchId(projectId, branchId) so
the cleanup logic is centralized and stays in sync.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/branching/BranchSnapshotService.kt`:
- Around line 32-42: Add a KDoc above the getBranchFilter function explaining
the default-branch convention: state that keys belonging to the default branch
are stored with branch_id == NULL and therefore when isDefault is true the SQL
must match either the given branch_id parameter or NULL; mention the expected
semantics and any callers that rely on this behavior (e.g., callers that pass
isDefault=true to include default-branch keys). Ensure the KDoc is concise and
placed immediately above the getBranchFilter function.
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt`:
- Around line 132-168: Extract the repeated setup in the test method `merge
activity includes source and target branch names in params` (the block using
`initConflicts()`, the `waitForNotThrowing` refresh/asserts, the
`createMergeWithConflict(...)` call, and the
`performProjectAuthPost("branches/merge/$mergeId/apply")`) into a helper like
`applyMergeWithConflicts(): Long` that returns the `mergeId`; update this test
and the other similar test (`merges resolved feature branch into main`) to call
that helper instead of duplicating the same lines, keeping the existing
polling/validation logic in the tests themselves.
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/ContentDeliveryConfigBranchingTest.kt`:
- Around line 160-170: The test uses a hard non-null assertion config.branch!!
which can throw an NPE and give poor failure context; change it to assert the
branch is present first and then use the asserted value (e.g. call
config.branch.assert.isNotNull() then assign val branch = config.branch!! or use
config.branch?.let { it.name.assert.isEqualTo("feature") }) so the test fails
with a clear assertion message instead of an NPE; update the test method CDN
config filterBranch is derived from branch entity
(ContentDeliveryConfigBranchingTest) to perform the null-check/assert before
accessing branch.name.
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/branching/BranchMergeServiceTest.kt`:
- Line 642: A local variable named `key` shadows the function parameter `key`;
rename the local `val key = keyService.getKeysWithTagsById(...)` to a
non-conflicting name (e.g., `keyWithTags` or `keyDto`) so references to the
parameter and the fetched object are unambiguous; update subsequent usages in
this test (the result of `getKeysWithTagsById`) to use the new identifier and
leave the method parameter `key` unchanged.
In `@webapp/src/views/projects/developer/contentDelivery/CdDialog.tsx`:
- Line 42: The file-level import "import React from 'react';" is unused in the
CdDialog component; remove that import statement from CdDialog.tsx (or replace
it with only the necessary type imports if you need types) so the module no
longer contains an unused React import.
…s `activityTestUtil` from BranchControllerMergingTest.kt
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/cypress/e2e/branching/merge.cy.ts (1)
173-178:⚠️ Potential issue | 🟡 MinorMissing
selectTabbeforeassertKeyInTab— inconsistent with all other test cases.Every other
assertKeyInTabcall in this file is preceded by an explicitselectTab(...). This test's mutation is aneditTranslationon'shared-update-key', so the key appears under the UPDATE tab. Without first navigating to that tab, the assertion targets whatever tab the merge UI opens to by default — making the precondition either unreliable or vacuously passing on the wrong tab.🛠 Proposed fix
mergeSection.initiateMergeFromBranches('feature'); + mergeSection.assertStats({ modifications: 1 }); + mergeSection.selectTab('UPDATE'); mergeSection.assertKeyInTab('shared-update-key');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/branching/merge.cy.ts` around lines 173 - 178, The test misses selecting the UPDATE tab before asserting the key; update the sequence around mergeSection.initiateMergeFromBranches('feature') so that you call mergeSection.selectTab('UPDATE') (or the existing selectTab enum/value used elsewhere) immediately before mergeSection.assertKeyInTab('shared-update-key'), ensuring the assertion targets the UPDATE tab; leave mergeSection.setDeleteBranchAfterMerge(true) as-is after the assertion.
🧹 Nitpick comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt (1)
128-141: Consider absorbing the revision-wait intoinitConflicts().Lines 132-141 are an exact duplicate of lines 99-108 in
merges resolved feature branch into main. Both tests callinitConflicts()and then wait forrevision > 0; moving that wait intoinitConflicts()(after thepending.isFalsegate) removes the duplication.♻️ Proposed refactor
In
initConflicts():waitForNotThrowing(timeout = 10000, pollTime = 250) { testData.featureBranch .refresh() .pending.assert.isFalse } + // wait for revision numbers to be updated after snapshot + key creation + waitForNotThrowing(timeout = 10000, pollTime = 250) { + testData.featureBranch.refresh().revision.assert.isGreaterThan(0) + testData.mainBranch.refresh().revision.assert.isGreaterThan(0) + } updateKeyTranslation(keys.first, "main translation")Then remove the now-duplicate blocks from both
merges resolved feature branch into mainand the new test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt` around lines 128 - 141, Move the redundant revision-wait logic into initConflicts(): after the existing pending.isFalse gate in initConflicts(), add the waitForNotThrowing block that refreshes testData.featureBranch and testData.mainBranch and asserts revision > 0; then remove the duplicated waitForNotThrowing blocks from both the `merges resolved feature branch into main` test and the `merge activity includes source and target branch names in params` test so they simply call initConflicts() and rely on it to guarantee revisions are populated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@e2e/cypress/e2e/branching/merge.cy.ts`:
- Around line 173-178: The test misses selecting the UPDATE tab before asserting
the key; update the sequence around
mergeSection.initiateMergeFromBranches('feature') so that you call
mergeSection.selectTab('UPDATE') (or the existing selectTab enum/value used
elsewhere) immediately before mergeSection.assertKeyInTab('shared-update-key'),
ensuring the assertion targets the UPDATE tab; leave
mergeSection.setDeleteBranchAfterMerge(true) as-is after the assertion.
---
Nitpick comments:
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/branching/BranchControllerMergingTest.kt`:
- Around line 128-141: Move the redundant revision-wait logic into
initConflicts(): after the existing pending.isFalse gate in initConflicts(), add
the waitForNotThrowing block that refreshes testData.featureBranch and
testData.mainBranch and asserts revision > 0; then remove the duplicated
waitForNotThrowing blocks from both the `merges resolved feature branch into
main` test and the `merge activity includes source and target branch names in
params` test so they simply call initConflicts() and rely on it to guarantee
revisions are populated.
bdshadow
approved these changes
Feb 19, 2026
TolgeeMachine
added a commit
that referenced
this pull request
Feb 19, 2026
This was referenced Mar 2, 2026
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates several branching-related improvements:
BranchMergeActivityParamsProvider(fix: use projection queries in BranchMergeActivityParamsProvider #3469)Latest changes
Summary by CodeRabbit
New Features
Tests