Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete QA checks feature: DB schema, domain models/repositories, batch job and processor, QA runner and checks, services/controllers/WebSocket preview, query/view/model extensions, frontend components/hooks, event listeners, and many tests and feature-flag integrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser as Client
participant WS as Browser WebSocket
participant WSHandler as QaCheckPreviewWSHandler
participant Runner as QaCheckRunnerService
participant Repo as TranslationQaIssueRepository
participant UI as Browser UI
Browser->>WS: open /ws/qa-preview, send init (token, projectId, keyId?, languageTag)
WS->>WSHandler: init -> authenticate & permission check, load persisted issues
Browser->>WS: send { text, variant }
WS->>WSHandler: text payload
WSHandler->>Runner: runEnabledChecks(projectId, params)
Runner->>Repo: read persisted issues (optional)
Runner-->>WSHandler: results per check type
WSHandler-->>WS: stream `result` messages
WSHandler-->>WS: send `done`
WS->>UI: deliver messages to client UI
sequenceDiagram
autonumber
participant TranslationSvc as TranslationService
participant Publisher as ApplicationEventPublisher
participant Listener as QaCheckTextsModifiedListener
participant BatchSvc as BatchJobService
participant Processor as QaCheckChunkProcessor
participant QaBatch as QaCheckBatchServiceImpl
participant Websocket as WebsocketPublisher
TranslationSvc->>Publisher: publish OnTranslationTextsModified(translationIds, projectId)
Publisher-->>Listener: event (BEFORE_COMMIT)
Listener->>BatchSvc: start hidden BatchJobType.QA_CHECK with targets
BatchSvc->>Processor: process chunk (keyId, languageId)
Processor->>QaBatch: runChecksAndPersist(projectId,keyId,languageId)
QaBatch-->>Processor: persist issues, set qaChecksStale=false
QaBatch->>Websocket: publish QA_ISSUES_UPDATED event
Websocket-->>TranslationSvc: frontend receives update via websocket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Anty0
force-pushed
the
jirikuchynka/qa-checks
branch
2 times, most recently
from
4a31c59 to
3a4fd39
Compare
Anty0
force-pushed
the
jirikuchynka/qa-checks
branch
4 times, most recently
from
95fc417 to
d83471a
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: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/data/src/main/kotlin/io/tolgee/service/project/ProjectService.kt (1)
195-204:⚠️ Potential issue | 🟠 MajorEmit the base-language change event on first assignment too.
Line 202 suppresses
OnProjectBaseLanguageChangedwhen the previous base language isnull. IneditProject(), that means a project going from “no base language” to a defined one never fans out to the QA/base-language listeners, even though the comparison source just changed.🛠️ Suggested fix
- if (oldBaseLanguageId != null && oldBaseLanguageId != language.id) { + if (oldBaseLanguageId != language.id) { applicationContext.publishEvent(OnProjectBaseLanguageChanged(project.id)) }🤖 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/project/ProjectService.kt` around lines 195 - 204, The current block in editProject handling dto.baseLanguageId updates project.baseLanguage but only publishes OnProjectBaseLanguageChanged when oldBaseLanguageId != null and changed; update the condition so the event is also published on first assignment (when oldBaseLanguageId is null) — i.e. after setting project.baseLanguage, call applicationContext.publishEvent(OnProjectBaseLanguageChanged(project.id)) when oldBaseLanguageId == null or oldBaseLanguageId != language.id; adjust the conditional that references oldBaseLanguageId and language.id accordingly to ensure first-time assignment triggers the event.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/eeSubscription/EeSubscriptionServiceImpl.kt (1)
219-236:⚠️ Potential issue | 🔴 CriticalMove the remote license release outside this transaction.
@Transactionalwrapsclient.releaseKeyRemote(subscription). If the remote release succeeds butdeleteAll()or the subsequent event publication fails, the transaction rolls back—but the cloud side has already freed the key. This creates divergence where the cloud considers the license released while the local instance still holds the subscription record. Additionally, this keeps the database transaction (and connection) open for the entire duration of the HTTP call.The exception handler only catches
NotFound; other errors will still trigger rollback after a successful remote release. Extract the remote call outside the transaction boundary.🤖 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/eeSubscription/EeSubscriptionServiceImpl.kt` around lines 219 - 236, The remote HTTP call client.releaseKeyRemote(subscription) must be moved outside the `@Transactional` boundary to avoid holding the DB transaction during the HTTP call and to prevent rollback divergence; change releaseSubscription() so it first locates the subscription via findSubscriptionEntity() in a non-transactional context, perform client.releaseKeyRemote(subscription) (with the same HttpClientErrorException.NotFound handling) before starting the transaction, and then execute the DB operations eeSubscriptionRepository.deleteAll() and publishFeaturesChangedIfNeeded(oldFeatures, emptyArray()) inside a `@Transactional` method (keeping `@CacheEvict` on the transactional delete/publish method); ensure oldFeatures is captured before the remote call or passed into the transactional block so the publish logic still has the previous features.
🟠 Major comments (32)
webapp/src/service/http/useQueryApi.ts-129-134 (1)
129-134:⚠️ Potential issue | 🟠 MajorAlign cache key composition across all query helpers to prevent cache collisions.
Lines 129-134 now include
request.contentin theuseApiQuerycache key, butuseApiInfiniteQuery(line 104) anduseApiQueries(line 159) do not. This inconsistency allows requests differing only by body to collide in the cache when using those helpers.Proposed fix
diff --git a/webapp/src/service/http/useQueryApi.ts b/webapp/src/service/http/useQueryApi.ts @@ - [url, (request as any)?.path, (request as any)?.query], + [ + url, + (request as any)?.path, + (request as any)?.query, + (request as any)?.content, + ], @@ - queryKey: [url, (request as any)?.path, (request as any)?.query], + queryKey: [ + url, + (request as any)?.path, + (request as any)?.query, + (request as any)?.content, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/service/http/useQueryApi.ts` around lines 129 - 134, The cache key for useApiQuery includes (request as any)?.content but useApiInfiniteQuery and useApiQueries do not, causing cache collisions for requests that differ only by body; update the cache key arrays in useApiInfiniteQuery and useApiQueries to include (request as any)?.content so all three helpers compose keys the same way (useApiQuery, useApiInfiniteQuery, useApiQueries) using [url, (request as any)?.path, (request as any)?.query, (request as any)?.content].ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaLanguageSettingsRequest.kt-7-11 (1)
7-11:⚠️ Potential issue | 🟠 MajorConvert to
data classwith property in primary constructor for proper JSON deserialization.The current class structure won't deserialize
settingsfrom the request body. With Jackson-Kotlin, JSON values are bound via the primary constructor; class-body properties without setters can't be populated. This means the API will always use the empty default, ignoring user input.All other Request DTOs in the codebase follow the
data classpattern with properties in the constructor. Apply the same pattern here:Proposed fix
-class QaLanguageSettingsRequest { - `@field`:Schema( - description = "Map of check types to their severity. Null values mean 'inherit from global settings'.", - ) - val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap() -} +data class QaLanguageSettingsRequest( + `@field`:Schema( + description = "Map of check types to their severity. Null values mean 'inherit from global settings'.", + ) + val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap(), +)🤖 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/data/qa/QaLanguageSettingsRequest.kt` around lines 7 - 11, Convert the class QaLanguageSettingsRequest into a data class and move the settings property into the primary constructor so Jackson-Kotlin can deserialize it; e.g. define data class QaLanguageSettingsRequest(val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap()) and retain the `@field`:Schema annotation on the constructor parameter to preserve the API docs/description for settings.backend/data/src/main/kotlin/io/tolgee/component/demoProject/DemoProjectCreator.kt-46-55 (1)
46-55:⚠️ Potential issue | 🟠 MajorPublish QA recheck event after project-created event.
On Line 46, QA recheck is published before
OnProjectCreated(Line 54). If QA processing depends on project-level QA enablement initialized by project-created listeners, this can skip the initial recheck for demo projects.Suggested reorder
setStates() - publishQaRecheckEvent() addBigMeta() addScreenshots() tagKeys() setDescriptions() addComments() project.baseLanguage = languages["en"] projectService.save(project) applicationContext.publishEvent(OnProjectCreated(project)) + publishQaRecheckEvent() return projectAlso applies to: 58-65
🤖 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/component/demoProject/DemoProjectCreator.kt` around lines 46 - 55, The QA recheck is published too early; move the call to publishQaRecheckEvent() so it runs after the project-created event is published. Specifically, after projectService.save(project) and applicationContext.publishEvent(OnProjectCreated(project)) complete, invoke publishQaRecheckEvent() (and do the same reordering for the similar block around the other instance at lines 58-65) to ensure any project-created listeners that initialize QA settings run before the recheck.webapp/src/ee/qa/hooks/useQaCheckTypeLabel.ts-10-44 (1)
10-44:⚠️ Potential issue | 🟠 MajorAdd
defaultValueto each QA label translation call.All
t()usages here omitdefaultValue, which breaks the repository's required fallback behavior before translations are loaded. According to the webapp translation guidelines, thet()function requires bothkeyNameanddefaultValueparameters.🛠️ Example fix pattern
- return t('qa_check_type_empty_translation'); + return t('qa_check_type_empty_translation', 'Empty translation'); ... - return t('qa_check_type_unmatched_newlines'); + return t('qa_check_type_unmatched_newlines', 'Unmatched newlines');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/hooks/useQaCheckTypeLabel.ts` around lines 10 - 44, The t() calls inside useQaCheckTypeLabel are missing defaultValue fallbacks; update every return that calls t('qa_check_type_*') (within the useQaCheckTypeLabel function) to pass a second argument defaultValue with the human-readable fallback string (e.g., the current key or a readable label) so each call becomes t('qa_check_type_xxx', { defaultValue: 'Your fallback' }); ensure every case (EMPTY_TRANSLATION, SPACES_MISMATCH, UNMATCHED_NEWLINES, CHARACTER_CASE_MISMATCH, MISSING_NUMBERS, SPELLING, GRAMMAR, REPEATED_WORDS, PUNCTUATION_MISMATCH, BRACKETS_MISMATCH, BRACKETS_UNBALANCED, SPECIAL_CHARACTER_MISMATCH, DIFFERENT_URLS, INCONSISTENT_PLACEHOLDERS, INCONSISTENT_HTML, HTML_SYNTAX, ICU_SYNTAX, KEY_LENGTH_LIMIT) is updated consistently.webapp/src/ee/qa/components/IssueRow.tsx-47-53 (1)
47-53:⚠️ Potential issue | 🟠 MajorUse a semantic button for the row.
Line 47 renders a clickable
div, so keyboard users can't focus or activate this entry, and the hover-only affordance never appears on focus.♿ Suggested fix
-import { Box, styled, Typography } from '@mui/material'; +import { ButtonBase, styled, Typography } from '@mui/material'; ... -const StyledRow = styled(Box)` +const StyledRow = styled(ButtonBase)` display: flex; align-items: center; justify-content: space-between; + width: 100%; padding: 4px 8px; border-radius: 4px; cursor: pointer; + text-align: left; - &:hover { + &:hover, + &:focus-visible { background: ${({ theme }) => theme.palette.tokens.text._states.hover}; } ... - &:hover .show-link { + &:hover .show-link, + &:focus-visible .show-link { visibility: visible; } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/components/IssueRow.tsx` around lines 47 - 53, The clickable row uses a non-interactive element (StyledRow) with onClick which is not keyboard accessible; change StyledRow into a semantic button (or render it as a <button> via your styled-component) and keep styles from StyledRow, ensure it is focusable and keyboard-activatable (handle Enter/Space activation if you use a custom element), keep the onClick handler, and ensure the existing child StyledShowLink and Typography remain unchanged; also add appropriate aria attributes if needed (e.g., aria-pressed/aria-label) so the row is accessible to keyboard and assistive tech.webapp/src/views/projects/DashboardProjectListItem.tsx-129-133 (1)
129-133:⚠️ Potential issue | 🟠 MajorFix filter semantics for stale-only QA badges and add missing default values for translations.
The badge shows when
qaIssueCount > 0ORqaChecksStaleCount > 0, but the filter navigates tofilterHasQaIssueswhich only matches actual issues (qaIssueCount > 0). Projects with stale checks only will show the badge but land on an empty list. Consider routing stale-only projects to either include stale checks in the filter or separate the navigation paths.Also, add
defaultValueto fourt()calls (lines 186, 189, 203, 206) per the Tolgee default-value rule:
t('project_list_qa_issues_button')t('project_list_translations_button')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/views/projects/DashboardProjectListItem.tsx` around lines 129 - 133, The QA badge shows when either hasQaIssues or hasStaleQaChecks is true (computed in showQaBadge) but the click navigates to the filter that only matches actual issues (filterHasQaIssues), causing stale-only projects to land on an empty list; update the click/navigation logic in DashboardProjectListItem so that when a project is stale-only (hasQaChecksStaleCount > 0 && qaIssueCount === 0) you either route to a filter that includes stale checks (e.g., add or use a filter flag for stale QA checks) or create a separate navigation path for stale-only projects, and ensure the badge click logic uses hasQaIssues and hasStaleQaChecks to choose the correct filter. Also add defaultValue to the four Tolgee t() calls referenced (include defaultValue for keys 'project_list_qa_issues_button' and 'project_list_translations_button' and the other two t() usages mentioned) so all t(...) calls provide defaultValue per the rule; update the t(...) invocations in this component accordingly.backend/data/src/main/kotlin/io/tolgee/model/qa/LanguageQaConfig.kt-22-26 (1)
22-26:⚠️ Potential issue | 🟠 MajorAdd nullable constraints to the
settingsfield and specifyoptional = falseon the@ManyToOnerelationship.The Liquibase migration already constrains
language_idtoNOT NULL, but thesettingsJSONB column remains nullable in the database. Update both the entity annotations and the Liquibase changelog to match the Kotlin non-null types and the intended design.🛠️ Suggested fixes
Entity (add missing constraints):
import jakarta.persistence.Column import jakarta.persistence.Entity import jakarta.persistence.Index +import jakarta.persistence.JoinColumn import jakarta.persistence.ManyToOne import jakarta.persistence.Table ... - `@ManyToOne` + `@ManyToOne`(optional = false) + `@JoinColumn`(nullable = false) var language: Language, `@Type`(JsonBinaryType::class) - `@Column`(columnDefinition = "jsonb") + `@Column`(columnDefinition = "jsonb", nullable = false) var settings: MutableMap<QaCheckType, QaCheckSeverity> = mutableMapOf(),Liquibase (add missing constraint to settings column in changeSet
id="1773270150000-1"):<column name="settings" type="JSONB"/> +<constraints nullable="false"/>🤖 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/qa/LanguageQaConfig.kt` around lines 22 - 26, The entity LanguageQaConfig must reflect non-null constraints: mark the `@ManyToOne` language relationship as optional = false and make the settings field non-nullable in the JPA mapping (e.g., add nullable = false constraint on the settings `@Column/`@Type mapping) so the Kotlin non-null types align with the DB; also update the Liquibase changelog changeSet id="1773270150000-1" to set the settings JSONB column constraints nullable="false" so the DB enforces the same non-null rule. Ensure you modify the LanguageQaConfig class (language and settings properties) and the specified changeSet to keep schema and entity consistent.webapp/src/views/projects/translations/translationVisual/TranslationVisual.tsx-50-53 (1)
50-53:⚠️ Potential issue | 🟠 MajorDon't short-circuit empty translations once QA issues exist.
With
qaIssuesnow flowing through this component,if (!text) return nullmeansEMPTY_TRANSLATIONand other zero-length QA markers never reachTranslationVariantVisual.getTolgeeFormat(text || '', ...)already handles empty content, so this should only bail out when there is neither text nor open issues to render.Suggested fix
- if (!text) { + if (!text && openedQaIssues.length === 0) { return null; }Also applies to: 63-65, 67-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/views/projects/translations/translationVisual/TranslationVisual.tsx` around lines 50 - 53, The component currently returns early when text is empty which prevents EMPTY_TRANSLATION and other zero-length QA markers from reaching TranslationVariantVisual; change the early-return checks (e.g., where it does `if (!text) return null`) to only bail out when there is neither text nor any open QA issues (use openedQaIssues derived from qaIssues), so keep rendering when openedQaIssues.length > 0 even if text is empty; apply the same conditional fix to all similar checks in this file (around the blocks referenced at lines 63-65 and 67-85) and rely on getTolgeeFormat(text || '', ...) to handle empty content while still passing EMPTY_TRANSLATION through to TranslationVariantVisual.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt-29-32 (1)
29-32:⚠️ Potential issue | 🟠 MajorMultiset comparison loses duplicate count information.
The expression
baseNumbers - textNumbers.toSet()convertstextNumbersto a Set, losing count information. If the base text contains"1, 1, 2"and the translation has only"1, 2", no issue will be reported even though one "1" is missing.Compare with
DifferentUrlsCheckwhich correctly handles multiset semantics usinggroupingBy { it }.eachCount().Proposed fix using multiset comparison
val baseNumbers = extractNumbers(base) val textNumbers = extractNumbers(text) - val missing = baseNumbers - textNumbers.toSet() + val baseMultiset = baseNumbers.groupingBy { it }.eachCount().toMutableMap() + val textMultiset = textNumbers.groupingBy { it }.eachCount() + + // Subtract text counts from base counts + for ((num, count) in textMultiset) { + baseMultiset[num] = (baseMultiset[num] ?: 0) - count + } + + val missing = baseMultiset.flatMap { (num, count) -> + if (count > 0) List(count) { num } else emptyList() + } return missing.map { number ->🤖 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/qa/checks/MissingNumbersCheck.kt` around lines 29 - 32, The current subtraction `baseNumbers - textNumbers.toSet()` in MissingNumbersCheck loses duplicate counts; replace it with multiset-aware logic similar to DifferentUrlsCheck: compute baseCounts = baseNumbers.groupingBy { it }.eachCount() and textCounts = textNumbers.groupingBy { it }.eachCount(), then build the missing list by including each number repeated max(0, baseCounts[number] - (textCounts[number] ?: 0)) times (or otherwise report numbers where baseCount > textCount). Update the code that uses `baseNumbers`, `textNumbers`, and `extractNumbers` in MissingNumbersCheck to use these count maps for correct multiset comparison.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/SpacesMismatchCheck.kt-47-47 (1)
47-47:⚠️ Potential issue | 🟠 MajorDon't flag doubled spaces that already exist in the source.
checkDoubledSpaces()only inspectstext, sobase = "foo bar"andtext = "foo bar"still emitQA_SPACES_DOUBLED. For a*_MISMATCHcheck that turns into a false positive on a valid translation.Also applies to: 92-114
🤖 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/qa/checks/SpacesMismatchCheck.kt` at line 47, The check is flagging doubled spaces even when the source/base already contains them; update SpacesMismatchCheck so checkDoubledSpaces considers the base text and only emits QA_SPACES_DOUBLED when the doubled-space instance is introduced or differs from base. Concretely, change checkDoubledSpaces to accept both base and text (e.g. checkDoubledSpaces(base, text, results)) and in its logic compare occurrences/positions of doubled spaces in text against base and only add a result when a doubled space exists in text but not at the same place in base; apply the same change to the other callsite(s) mentioned (lines 92-114) so all doubled-space checks use base vs text comparison.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/BracketsMismatchCheck.kt-45-52 (1)
45-52:⚠️ Potential issue | 🟠 MajorUse an empty replacement for extra brackets.
These results already point at an exact span, but
replacement = nullgives the CORRECT action nothing to apply. The other “extra token” checks use""for this delete case, and this one should too.Suggested fix
QaCheckResult( type = QaCheckType.BRACKETS_MISMATCH, message = QaIssueMessage.QA_BRACKETS_EXTRA, - replacement = null, + replacement = "", positionStart = pos, positionEnd = pos + 1, params = mapOf("bracket" to bracket.toString()), ),🤖 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/qa/checks/BracketsMismatchCheck.kt` around lines 45 - 52, In BracketsMismatchCheck where a QaCheckResult is created for extra brackets, set replacement to an empty string instead of null so the "CORRECT" action can apply a delete; update the QaCheckResult instantiation in the BracketsMismatchCheck (the entry that currently uses replacement = null and params = mapOf("bracket" to bracket.toString())) to use replacement = "" to match other extra-token checks.webapp/src/views/projects/translations/translationVisual/TranslationWithPlaceholders.tsx-112-123 (1)
112-123:⚠️ Potential issue | 🟠 MajorSort modifiers by both start and end position to prevent text duplication.
When multiple modifiers share the same start offset (e.g., a zero-length QA marker and a placeholder both starting at position 6), sorting only by
startleaves their processing order non-deterministic. If the longer modifier is processed first, it advancesindexpast the zero-length marker's start position, causing the subsequent rendering loop to generate backward text slices (e.g.,text[11:6]) and duplicate content.Sorting secondarily by
endposition ensures shorter ranges process first, preventing backwards slicing and text duplication.Proposed fix
return modifiers .filter( ({ position }) => position.start <= contentLength && position.end >= 0 ) .map((modifier) => ({ ...modifier, position: { start: Math.max(0, modifier.position.start), end: Math.min(contentLength, modifier.position.end), }, })) - .sort((a, b) => a.position.start - b.position.start); + .sort( + (a, b) => + a.position.start - b.position.start || + a.position.end - b.position.end + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/views/projects/translations/translationVisual/TranslationWithPlaceholders.tsx` around lines 112 - 123, The current sort in TranslationWithPlaceholders.tsx only orders modifiers by start which leaves ties ambiguous and can cause backward slicing and duplicated text; update the sort comparator used after mapping (the .sort(...) call on modifiers) to break ties by end position as well so modifiers with the same start are ordered by their end (shorter ranges first) — implement a two-key numeric comparison using a.position.start vs b.position.start and, when equal, a.position.end vs b.position.end.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/lines/UnmatchedNewlinesCheck.kt-41-47 (1)
41-47:⚠️ Potential issue | 🟠 MajorCRLF positions are wrong for extra-newline fixes.
diffis a count of blank lines, butpositionStart = textGap.endIndex - diffsubtracts characters. With\r\n, that range removes only half of the newline sequence and leaves a stray\r. Use the detected separator length when computing the start offset.🐛 Proposed fix
- results.add(extraNewlinesResult(baseGap, textGap)) + results.add( + extraNewlinesResult(baseGap, textGap, textStructure.separatorType), + ) ... private fun extraNewlinesResult( baseGap: Gap, textGap: Gap, + separatorType: SeparatorType, ): QaCheckResult { val diff = textGap.lineCount - baseGap.lineCount + val extraLength = separatorType.separator.length * diff return QaCheckResult( type = QaCheckType.UNMATCHED_NEWLINES, message = QaIssueMessage.QA_NEWLINES_EXTRA, replacement = "", - positionStart = textGap.endIndex - diff, + positionStart = textGap.endIndex - extraLength, positionEnd = textGap.endIndex, params = mapOf("count" to diff.toString()), ) }Also applies to: 75-85
🤖 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/qa/checks/lines/UnmatchedNewlinesCheck.kt` around lines 41 - 47, The calculation of positionStart in UnmatchedNewlinesCheck (used by extraNewlinesResult/missingNewlinesResult) subtracts diff (a line count) from textGap.endIndex treating it like characters, which breaks CRLF sequences by removing only '\n'; change the offset computation to multiply the blank-line diff by the detected separator length (use textStructure.separatorType or its length) when computing positionStart and any range removals so you remove full newline sequences; apply the same adjustment in the other symmetric block (lines ~75-85) that computes ranges for fixes.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaCheckBatchServiceImpl.kt-48-71 (1)
48-71:⚠️ Potential issue | 🟠 MajorDon't persist QA results from a stale snapshot.
The check inputs are read before
executeInNewRepeatableTransaction(...), but the write later updates the current translation and marks it fresh. If the target text or base text changes while the checks are running, you'll store issues for the old snapshot withqaChecksStale = false. Re-read or version-check the source texts inside the write transaction and skip/requeue when they differ.Also applies to: 96-103
🤖 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/qa/QaCheckBatchServiceImpl.kt` around lines 48 - 71, The reads for translation/base text (existingTranslation from translationRepository.findOneByProjectIdAndKeyIdAndLanguageId, baseText from translationService.getTranslations, and related computed values like translationText/textParsed/baseParsed) are done before executeInNewRepeatableTransaction(...) so you must re-fetch and verify these values inside the new transaction (or compare a version/timestamp) before writing QA results; update QaCheckBatchServiceImpl to, inside executeInNewRepeatableTransaction, reload the current translation and base language translation (using the same repository/service methods) and if any text/version differs from the pre-read snapshot, skip or requeue the QA result (set qaChecksStale=true or abort the write) instead of persisting stale issues.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/HtmlSyntaxCheck.kt-27-63 (1)
27-63:⚠️ Potential issue | 🟠 MajorA per-tag stack misses crossed tags.
Tracking open tags by name lets invalid nesting like
<b><i></b></i>drain both stacks and return no issue. For HTML syntax you need one LIFO stack of open tags, and each close tag should match the current top.🤖 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/qa/checks/HtmlSyntaxCheck.kt` around lines 27 - 63, The current HtmlSyntaxCheck.kt uses openTags (a map of tag name → stack) which allows crossed tags like <b><i></b></i> to pass; replace that with a single LIFO stack (e.g., a MutableList<HtmlTag> called openStack) and push on HtmlTagKind.OPEN (except VOID_ELEMENTS and SELF_CLOSING), pop and check the top on HtmlTagKind.CLOSE: if the top's name matches the closing tag, pop it; if it doesn't match or the stack is empty, add the QaCheckResult error (use the same QaCheckType/QaIssueMessage and tag.raw/positions) to report mismatched/unopened closing tags, and after processing tags report any remaining unclosed tags from the openStack.webapp/src/views/projects/translations/translationVisual/QaIssueHighlight.tsx-63-77 (1)
63-77:⚠️ Potential issue | 🟠 MajorReport the event only after
correctTranslation()succeeds.
correctTranslation()is an async function, but the handler fires it without awaiting and immediately reportsQA_ISSUE_CORRECTED_HIGHLIGHT. This causes failed corrections to be incorrectly counted as successful in analytics, and the rejected promise is left unhandled. Await the call and emit the event only on success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/views/projects/translations/translationVisual/QaIssueHighlight.tsx` around lines 63 - 77, The click handler handleCorrect currently calls the async correctTranslation(...) without awaiting and immediately calls reportEvent, causing unhandled rejections and false success analytics; update handleCorrect (in QaIssueHighlight) to be an async function, await correctTranslation({ translationId, translationText, issue: { positionStart, positionEnd, replacement } }), and only call reportEvent('QA_ISSUE_CORRECTED_HIGHLIGHT', { checkType: issue.type }) after the await succeeds; wrap the await in a try/catch to handle/log failures (and avoid emitting the event on error).ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/PunctuationMismatchCheck.kt-29-36 (1)
29-36:⚠️ Potential issue | 🟠 MajorCompare the full trailing punctuation suffix, not just the last character.
This collapses both sides to one char from
PUNCTUATION_CHARS, so"Hello..."vs"Bonjour."passes and"Hello?!"vs"Bonjour?"gets the wrong autofix. The check should compare the whole trailing punctuation run (and ideally include…) so the reported span and replacement stay correct.Also applies to: 38-82, 85-90
🤖 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/qa/checks/PunctuationMismatchCheck.kt` around lines 29 - 36, The code currently compares only a single final punctuation char (collapsing to one from PUNCTUATION_CHARS) which makes trailing runs like "..." or "?!", and the ellipsis, be treated incorrectly; update the logic in PunctuationMismatchCheck (look around variables/functions baseTrimmed, textTrimmed, trailingPunctuation and the blocks covering lines ~29–90) to extract and compare the entire trailing punctuation run from each string (not just the last char), include the unicode ellipsis (…) in the punctuation set, and use that full suffix when building the reported span and the autofix replacement so the span and replacement text remain correct for multi-char punctuation runs.webapp/src/fixtures/qaUtils.ts-11-22 (1)
11-22:⚠️ Potential issue | 🟠 MajorClamp and validate QA spans before slicing.
slice()treats negative indices as offsets from the end, so a malformed QA range can rewrite the tail oftextinstead of becoming a no-op. Since this helper sits on the mutation path after backend/variant remapping, it should clamp to[0, text.length]and ignorestart > end.Suggested hardening
export function applyQaReplacement( text: string, issue: QaReplacementParams ): string { if (issue.positionStart == null || issue.positionEnd == null) { return text; } + const start = Math.max(0, Math.min(issue.positionStart, text.length)); + const end = Math.max(0, Math.min(issue.positionEnd, text.length)); + if (start > end) { + return text; + } return ( - text.slice(0, issue.positionStart) + + text.slice(0, start) + (issue.replacement ?? '') + - text.slice(issue.positionEnd) + text.slice(end) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/fixtures/qaUtils.ts` around lines 11 - 22, The applyQaReplacement function must validate and clamp the span before slicing: ensure issue.positionStart and issue.positionEnd are numbers, clamp both to the range [0, text.length] (e.g., using Math.max/Math.min) and treat any start > end as a no-op (return original text); then perform slicing with the clamped indices and use (issue.replacement ?? '') for insertion. This prevents negative or out-of-range indices from being interpreted by String.prototype.slice and avoids unintended tail rewrites.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/HtmlTagParser.kt-135-144 (1)
135-144:⚠️ Potential issue | 🟠 MajorDon't swallow arbitrary characters after the tag name.
In
STATE_PARAMS_OR_END, every non-name/non-//non->character is silently ignored. That makes malformed input like<b ? >parse as a valid tag, which can suppress the HTML syntax QA check instead of reporting it. Only whitespace should be skipped here.Suggested fix
STATE_PARAMS_OR_END -> { when { isTagNameChar(cp) -> state = STATE_PARAM_NAME char == '/' -> { kind = HtmlTagKind.SELF_CLOSING state = STATE_SELF_CLOSING_END } char == '>' -> state = STATE_TAG_VALID - // else: skip whitespace and other chars + isWhiteSpace(cp) -> { /* skip */ } + else -> break@loop } }🤖 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/qa/checks/HtmlTagParser.kt` around lines 135 - 144, In the STATE_PARAMS_OR_END branch, don't silently ignore every non-name/non-'/'/non-'>' character; only skip whitespace. Update the logic in HtmlTagParser so that when in STATE_PARAMS_OR_END (the branch using isTagNameChar, STATE_PARAM_NAME, '/' -> STATE_SELF_CLOSING_END, and '>' -> STATE_TAG_VALID) you explicitly check for Character.isWhitespace(char) to continue, and otherwise transition the parser into a failure/invalid-tag state (or set a flag/error) instead of swallowing the character so malformed tags like "<b ? >" are reported.webapp/src/ee/qa/components/QaChecksPanel.tsx-153-156 (1)
153-156:⚠️ Potential issue | 🟠 MajorDon't render an ignore action that silently no-ops without a translation id.
For key/language pairs that still lack a persisted
Translation,handleIgnoreToggle()just returns, but every item still getsonIgnore. That gives users a clickable action with no effect.💡 Minimal fix to hide the broken action
const handleCorrect = (issue: (typeof issues)[0]) => { data.setValue(applyQaReplacement(text, issue)); reportEvent('QA_ISSUE_CORRECTED_INLINE', { checkType: issue.type }); }; - const handleIgnoreToggle = (issue: (typeof issues)[0]) => { - const translationId = data.keyData.translations[data.language.tag]?.id; + const translationId = data.keyData.translations[data.language.tag]?.id; + + const handleIgnoreToggle = (issue: (typeof issues)[0]) => { if (translationId == null) return; const newState = issue.state === 'IGNORED' ? 'OPEN' : 'IGNORED';{issues.map((issue, index) => ( <QaCheckItem key={`${issue.type}-${index}`} issue={issue} index={index + 1} text={text} slim={true} onCorrect={ isCorrectable(issue) ? () => handleCorrect(issue) : undefined } - onIgnore={() => handleIgnoreToggle(issue)} + onIgnore={ + translationId != null ? () => handleIgnoreToggle(issue) : undefined + } /> ))}Also applies to: 185-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/components/QaChecksPanel.tsx` around lines 153 - 156, The ignore action is rendered even when handleIgnoreToggle silently returns due to missing translationId; update the rendering logic so the onIgnore prop (or the ignore action/button) is only passed/shown when a persisted Translation exists: check the same condition you use in handleIgnoreToggle (const translationId = data.keyData.translations[data.language.tag]?.id) and only supply onIgnore={() => handleIgnoreToggle(issue)} or render the ignore control when translationId != null; apply the same guard to the other ignore handler block referenced (lines ~185-195) so users don't see a non-functional ignore action.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/DifferentUrlsCheck.kt-105-109 (1)
105-109:⚠️ Potential issue | 🟠 MajorDon't cut off URLs at closing brackets.
The current match stops at
)/]/}, so valid URLs likehttps://en.wikipedia.org/wiki/Function_(mathematics)are extracted without the closing bracket. That will generate falseQA_URL_*findings and incorrect replacements. Match the full token first and strip only unmatched trailing delimiters afterward.Also applies to: 119-126
🤖 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/qa/checks/DifferentUrlsCheck.kt` around lines 105 - 109, The URL_REGEX in DifferentUrlsCheck currently stops before closing delimiters (e.g., ), ], }) causing URLs like https://en.wikipedia.org/wiki/Function_(mathematics) to be cut off; update the matching strategy in the DifferentUrlsCheck class by expanding URL_REGEX to capture the full token (including possible trailing punctuation/brackets) and then, in the URL extraction logic (the code around URL_REGEX usage at lines referenced and the block around 119-126), post-process each match to strip only unmatched trailing delimiters and punctuation (e.g., . , ; : ! ? ' " ) ] }) ) while preserving balanced closing brackets that are part of the URL; ensure you reference the URL_REGEX constant and the URL extraction/normalization code so replacements/QA_URL_* findings use the corrected, fully-captured URL.ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaPreviewWebSocketTestHelper.kt-67-87 (1)
67-87:⚠️ Potential issue | 🟠 MajorTrack messages per round-trip instead of querying the whole deque.
waitForDone(),collectResults(),collectAllIssues(), andgetError()all read the entire message history. After a secondsendText()on the same socket,waitForDone()can match the previous"done"frame andcollectResults()will mix old and new issues. That makes the helper flaky for the multi-update flow it exposes.Also applies to: 96-133
🤖 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/qa/QaPreviewWebSocketTestHelper.kt` around lines 67 - 87, The helper currently scans the entire message history in waitForDone(), collectResults(), collectAllIssues(), and getError(), causing stale frames from prior sendText()/sendInit() calls to be mixed into new rounds; change the flow to track messages per round-trip by capturing the current message count/index before sending (in sendText() and sendInit()) and then have waitForDone()/collectResults()/collectAllIssues()/getError() only inspect messages appended after that saved index (or return a per-send future/completion marker from send/sendText that those collectors use) so each send only processes responses for that specific round.webapp/src/ee/qa/components/QaChecksPanel.tsx-59-63 (1)
59-63:⚠️ Potential issue | 🟠 MajorDon't start QA preview work when QA is disabled.
useQaChecksForPanel(data)runs before theQA_CHECKS/project.useQaChecksguards, and that hook enables preview wheneverisModified || qaChecksStale. So this panel can still create preview traffic for users who immediately get the disabled banner.Also applies to: 97-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/components/QaChecksPanel.tsx` around lines 59 - 63, The panel currently calls useQaChecksForPanel(data) unconditionally which lets that hook start preview work before QA is disabled checks run; update the code so the hook does not start preview when QA is disabled by either (A) changing useQaChecksForPanel to accept an enabled flag (e.g. useQaChecksForPanel(data, { enabled: isEnabled && project.useQaChecks }) and early-return/no-op when disabled), or (B) add a guard inside useQaChecksForPanel that reads useEnabledFeatures()/project.useQaChecks and prevents preview/start effects when those are false; reference QaChecksPanel, useQaChecksForPanel, useEnabledFeatures, and useProject when making the change.backend/data/src/main/kotlin/io/tolgee/service/queryBuilders/translationViewBuilder/QueryTranslationFiltering.kt-106-133 (1)
106-133:⚠️ Potential issue | 🟠 MajorAdd language scoping to the
filterQaCheckTypebranch for consistency with other filters.The
filterQaCheckTypebranch does not checklanguage.tag, breaking the pattern established by all other filters in this class (applyStateFilter,applyCommentsFilter,applySuggestionsFilter,applyOutdatedFilter). SinceapplyQaFilter()is called once per language in the query loop, the unscoped QA type filter will be AND-ed across all languages, causing the condition to apply globally rather than per-language. The first branch (filterHasQaIssuesInLang) correctly gates onlanguage.tag; the second branch should do the same.🤖 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/queryBuilders/translationViewBuilder/QueryTranslationFiltering.kt` around lines 106 - 133, The filterQaCheckType branch in applyQaFilter lacks per-language scoping and must only run for the current language; wrap or guard the params.filterQaCheckType logic with the same language check used earlier (e.g. require params.filterHasQaIssuesInLang?.contains(language.tag) == true before building the TranslationQaIssue subquery). Ensure the subquery still uses TranslationQaIssue, translation.get(Translation_.id), and checks state/type as before, but only when the language.tag is included for this language.ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaCheckPreviewWebSocketHandler.kt-187-195 (1)
187-195:⚠️ Potential issue | 🟠 MajorRefresh
translationIdafter a translation is created.Lines 187-195 capture
translationIdonce during init, and Lines 237-240 reuse that snapshot for every preview run. In the new key+language flow, a socket can start before aTranslationexists and continue after the first save; this code will keep returning no persisted issues, so preview state never reflects persistedOPEN/IGNOREDvalues until reconnect.Suggested fix
+ private fun resolveTranslationId(state: QaPreviewWsSessionState): Long? { + state.translationId?.let { return it } + val keyId = state.keyId ?: return null + val language = languageService.findByTag(state.languageTag, state.projectId) ?: return null + return translationService + .getTranslations(listOf(keyId), listOf(language.id)) + .firstOrNull() + ?.id + } + private fun fetchPersistedIssues(state: QaPreviewWsSessionState): List<TranslationQaIssue> { - val translationId = state.translationId ?: return emptyList() + val translationId = resolveTranslationId(state) ?: return emptyList() return qaIssueService.getIssuesForTranslation(state.projectId, translationId) }Also applies to: 237-240
🤖 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/api/v2/controllers/qa/QaCheckPreviewWebSocketHandler.kt` around lines 187 - 195, The code captures translationId once (variable translationId) during initialization via translationService.getTranslations and then reuses that stale value for every preview run, causing persisted OPEN/IGNORED states to be missed when a Translation is created after socket start; fix by re-querying translation existence on each preview execution instead of reusing the initial snapshot—call translationService.getTranslations (or the appropriate service method to fetch by keyId and language.id) inside the preview run path in QaCheckPreviewWebSocketHandler (replace the reuse at the later site currently using the initial translationId) so translationId is refreshed before checking persisted issues.webapp/src/ee/qa/hooks/useQaPreviewWebsocket.ts-47-57 (1)
47-57:⚠️ Potential issue | 🟠 MajorAvoid sending the same text twice on connect.
When
textis already available, Lines 54-57 send it fromonopen, and Lines 117-121 queue the same payload again. That doubles QA work and the firstdonecan clearisLoadingwhile the duplicate run is still in flight.Suggested fix
ws.onopen = () => { setIsLoading(false); // auth message ws.send( JSON.stringify({ token: jwtToken, projectId, keyId, languageTag }) ); // initial text update message - if (text != null) { + if (!pendingTextUpdateRef.current && text != null) { ws.send(JSON.stringify({ text, variant })); setIsLoading(true); } };Also applies to: 116-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/hooks/useQaPreviewWebsocket.ts` around lines 47 - 57, The ws.onopen handler in useQaPreviewWebsocket currently sends the initial text and the code later enqueues the same payload again (doubling work); modify the logic so the initial text is only sent once by adding a simple guard (e.g., an initialTextSent boolean or compare lastSentText) shared between ws.onopen and the enqueue path that sends text (the block that uses text and variant to queue/send later), and ensure setIsLoading is toggled only when that guarded send actually occurs; update the flag when sending so the other path skips sending the duplicate payload.backend/data/src/main/kotlin/io/tolgee/repository/qa/TranslationQaIssueRepository.kt-101-125 (1)
101-125:⚠️ Potential issue | 🟠 Major
findBy...IssueParamsstill dropsparams.Lines 101-125 make suppress/unignore lookups ambiguous because the query never compares the stored
params. Any two issues sharing type/message/replacement/position/pluralVariant collapse to the same row even if their payload differs, so ignore/unignore can hit the wrong issue.🤖 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/repository/qa/TranslationQaIssueRepository.kt` around lines 101 - 125, The query in findByProjectIdAndTranslationIdAndIssueParams is missing comparison of the stored params, causing different issues with the same type/message/etc. to collide; update the JPQL to include a params comparison like (i.params = :params or (i.params is null and :params is null)) and add a params: String? parameter to the function signature (TranslationQaIssueRepository.findByProjectIdAndTranslationIdAndIssueParams) so suppress/unignore operations match the exact issue payload.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.kt-49-67 (1)
49-67:⚠️ Potential issue | 🟠 MajorReusing previous issue state needs the full identity.
Lines 51-58 preserve the old state without checking
replacementorparams. That lets an ignored issue survive onto a different rerun result at the same location/message, which breaks suppression semantics.Suggested fix
val matchingExisting = existingIssues.find { existing -> existing.type == result.type && existing.message == result.message && + existing.replacement == result.replacement && existing.positionStart == result.positionStart && existing.positionEnd == result.positionEnd && + deserializeParams(existing.params) == result.params && existing.pluralVariant == result.pluralVariant }🤖 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/qa/QaIssueService.kt` around lines 49 - 67, The matchingExisting predicate in QaIssueService that builds TranslationQaIssue is too lax and preserves previous state without comparing the full identity; update the existingIssues.find lambda to also compare replacement and params (and ensure params comparison uses the same serialized form as stored on TranslationQaIssue, e.g. compare existing.params to objectMapper.writeValueAsString(result.params) or handle nulls consistently) so that matchingExisting only returns a previous issue when type, message, positionStart, positionEnd, pluralVariant, replacement and params all match.webapp/src/ee/qa/hooks/useQaPreviewWebsocket.ts-60-90 (1)
60-90:⚠️ Potential issue | 🟠 MajorIgnore callbacks from stale websocket instances.
Lines 60-90 mutate
issuesByType,isLoading, andwsRef.currentfor whichever socket fires the callback. After a reconnect, the previous socket can still deliverresult/done/close, which lets it overwrite the new preview state or clear the active ref.Suggested fix
const ws = new WebSocket(wsUrl); wsRef.current = ws; + const isCurrentSocket = () => wsRef.current === ws; setIsLoading(true); ws.onopen = () => { setIsLoading(false); @@ }; ws.onmessage = (event) => { + if (!isCurrentSocket()) return; const data: WsMessage = JSON.parse(event.data); if (data.type === 'result') { setIssuesByType((prev) => { const next = new Map(prev); @@ }; ws.onerror = () => { + if (!isCurrentSocket()) return; setIsLoading(false); }; ws.onclose = () => { - wsRef.current = null; + if (isCurrentSocket()) { + wsRef.current = null; + } }; return () => { ws.close(); - wsRef.current = null; + if (isCurrentSocket()) { + wsRef.current = null; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/ee/qa/hooks/useQaPreviewWebsocket.ts` around lines 60 - 90, The event handlers on ws (ws.onmessage, ws.onerror, ws.onclose and the cleanup) can act on stale sockets and mutate state for a newly connected socket; guard each handler and the cleanup so they only run if the socket instance is the active one (compare wsRef.current === ws before calling setIssuesByType, setIsLoading, mutating wsRef.current, or closing). Specifically, in useQaPreviewWebsocket wrap ws.onmessage logic (including the 'result'/'done'/'error' branches), ws.onerror and ws.onclose handlers with an early return unless wsRef.current === ws, and in the returned cleanup only call ws.close() and null out wsRef.current if wsRef.current === ws to avoid affecting a later connection.ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/ProjectQaConfigService.kt-33-48 (1)
33-48:⚠️ Potential issue | 🟠 MajorDon't limit the enable-time sweep to stale translations.
While QA is disabled, the update/delete methods skip rechecks entirely and do not mark anything stale. Re-enabling with
onlyStale = truetherefore misses translations whose effective settings changed during the disabled period but still haveqaChecksStale = false. Please either mark affected pairs stale while disabled or do a full recheck when turning QA back on.Also applies to: 98-157
🤖 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/qa/ProjectQaConfigService.kt` around lines 33 - 48, The enable flow in ProjectQaConfigService.setQaEnabled currently calls qaRecheckService.recheckTranslations(projectId, onlyStale = true) which misses translations that became out-of-date while QA was disabled; change the behavior so that when enabling QA (in setQaEnabled) you trigger a full recheck (call qaRecheckService.recheckTranslations(projectId, onlyStale = false) or equivalent) or alternatively ensure update/delete paths mark affected translation pairs qaChecksStale = true while QA is disabled—update setQaEnabled, qaRecheckService.recheckTranslations invocation, or the update/delete logic so re-enabling QA always re-evaluates all affected translations.webapp/src/eeSetup/eeModule.ee.tsx-518-559 (1)
518-559:⚠️ Potential issue | 🟠 MajorMove
useRouteMatchcalls to the top level of the hook.The code calls
useRouteMatch()inside the function returned fromuseAddProjectSettingsTabs(lines 531 and 552), which violates the Rules of Hooks. React hooks must be called at the top level of custom hooks or components, not inside nested callback functions. Call bothuseRouteMatch()hooks at the top level ofuseAddProjectSettingsTabsand pass the results into the returned function.Suggested fix
export const useAddProjectSettingsTabs = (projectId: number) => { const { t } = useTranslate(); + const qaRouteMatch = useRouteMatch(LINKS.PROJECT_EDIT_QA.template); + const labelsRouteMatch = useRouteMatch(LINKS.PROJECT_EDIT_LABELS.template); const tabsAdder = createAdder<ProjectSettingsTab>({ referencingProperty: 'value', }); @@ link: LINKS.PROJECT_EDIT_QA.build({ [PARAMS.PROJECT_ID]: projectId, }), dataCy: 'project-settings-menu-qa', component: ProjectSettingsQa, enabled: true, - routeMatch: useRouteMatch(LINKS.PROJECT_EDIT_QA.template), + routeMatch: qaRouteMatch, }, @@ link: LINKS.PROJECT_EDIT_LABELS.build({ [PARAMS.PROJECT_ID]: projectId, }), dataCy: 'project-settings-menu-labels', component: ProjectSettingsLabels, enabled: true, - routeMatch: useRouteMatch(LINKS.PROJECT_EDIT_LABELS.template), + routeMatch: labelsRouteMatch, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/eeSetup/eeModule.ee.tsx` around lines 518 - 559, The useRouteMatch calls inside the function returned by useAddProjectSettingsTabs violate the Rules of Hooks; move both useRouteMatch(LINKS.PROJECT_EDIT_QA.template) and useRouteMatch(LINKS.PROJECT_EDIT_LABELS.template) to the top level of the useAddProjectSettingsTabs hook (before returning the inner function), store their results in local constants (e.g., qaRouteMatch and labelsRouteMatch), and then use those constants when building the tabs (set routeMatch: qaRouteMatch for the ProjectSettingsQa tab and routeMatch: labelsRouteMatch for the ProjectSettingsLabels tab) so hooks are only invoked at the top level and the returned callback simply references the prepared values.webapp/src/service/apiSchema.generated.ts-5731-5734 (1)
5731-5734:⚠️ Potential issue | 🟠 MajorAllow
nullin per-language QA settings overrides.Line 5732 documents
nullas the way to inherit from global settings, but Line 5733 narrows every value to"WARNING" | "OFF". That means the generated client cannot represent the documented payload for clearing a language-specific override without unsafe casts. Please fix the OpenAPI nullability at the source and regenerate this file.Suggested type shape
QaLanguageSettingsRequest: { /** `@description` Map of check types to their severity. Null values mean 'inherit from global settings'. */ - settings: { [key: string]: "WARNING" | "OFF" }; + settings: { [key: string]: "WARNING" | "OFF" | null }; };Based on learnings: After backend API changes, regenerate TypeScript types by running
npm run schemain webapp directory (ensure backend is running first)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/service/apiSchema.generated.ts` around lines 5731 - 5734, The generated type for QaLanguageSettingsRequest.settings incorrectly forbids nulls; update the OpenAPI schema for the QaLanguageSettingsRequest model so the map's values allow null (e.g., value enum includes "WARNING" | "OFF" and is nullable or a oneOf including null), then regenerate the TypeScript types (run the schema generation command) so the client can represent per-language overrides being cleared; look for the QaLanguageSettingsRequest definition and the settings property to apply this change and re-run npm run schema to produce the corrected apiSchema.generated.ts.
| class QaEnabledRequest { | ||
| val enabled: Boolean = false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'class QaEnabledRequest|data class QaEnabledRequest|val enabled' ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaEnabledRequest.kt
rg -n -C4 'setQaEnabled\s*\(|QaEnabledRequest' ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.ktRepository: tolgee/tolgee-platform
Length of output: 1185
enabled cannot deserialize from JSON with current DTO shape.
Line 3–4 defines a regular Kotlin class with an immutable val enabled field initialized outside the constructor. Jackson cannot set immutable fields after object construction, so enabled will always remain false regardless of request payload. Convert to a data class with a constructor parameter so Jackson can populate the field during initialization.
Proposed fix
-class QaEnabledRequest {
- val enabled: Boolean = false
-}
+data class QaEnabledRequest(
+ val enabled: Boolean = false,
+)📝 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.
| class QaEnabledRequest { | |
| val enabled: Boolean = false | |
| data class QaEnabledRequest( | |
| val enabled: Boolean = false, | |
| ) |
🤖 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/data/qa/QaEnabledRequest.kt`
around lines 3 - 4, QaEnabledRequest currently defines an immutable val enabled
initialized outside the constructor which prevents Jackson from deserializing
request JSON into it; change QaEnabledRequest into a Kotlin data class with
enabled as a constructor parameter (e.g. data class QaEnabledRequest(val
enabled: Boolean)) so Jackson can populate the field during object construction
and requests reflect the payload.
There was a problem hiding this comment.
That doesn't sound right. I was able to turn on/off QA checks when testing locally. Please recheck.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show QaEnabledRequest.kt ==="
sed -n '1,200p' ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaEnabledRequest.kt || true
echo
echo "=== Find usages of QaEnabledRequest ==="
rg -n -C2 'QaEnabledRequest' || true
echo
echo "=== Show QaSettingsController.kt around setQaEnabled ==="
rg -n 'setQaEnabled' ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.kt || true
sed -n '1,260p' ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.kt || true
echo
echo "=== Look for `@RequestBody` on setQaEnabled parameter ==="
rg -n -C3 '@PutMapping\(.*/qa-settings/enabled.*\)|@RequestBody|QaEnabledRequest' ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.kt || true
echo
echo "=== Search tests or callers hitting /qa-settings/enabled ==="
rg -n -C3 '/qa-settings/enabled|\bsetQaEnabled\(' || true
Length of output: 14992
@Anty0 Thanks for flagging this. I rechecked the code and the call path:
- DTO: ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaEnabledRequest.kt currently is
class QaEnabledRequest { val enabled: Boolean = false } - Controller: setQaEnabled(
@RequestBodydto: QaEnabledRequest) uses dto.enabled. - Frontend sends PUT /v2/projects/{projectId}/qa-settings/enabled with JSON { enabled: true|false }.
Why it worked for you locally: Jackson can bind via field access and, depending on mapper settings, it will set instance fields by reflection even if they’re declared val (final). That’s likely what you observed, so the endpoint toggles as expected in your environment.
That said, it’s not idiomatic Kotlin for request DTOs and it relies on reflective writes to a final field. To make this robust across mapper/JDK configurations and consistent with typical Kotlin + Jackson usage, I’d still suggest converting it to a constructor-backed DTO (or using var):
Kotlin
data class QaEnabledRequest(
val enabled: Boolean = false,
)
If we touch this, it would also be good to align QaSettingsRequest and QaLanguageSettingsRequest the same way for consistency. Not a blocker given your local verification, but a small hardening/cleanup that avoids surprises later.
| class QaSettingsRequest { | ||
| val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== QaSettingsRequest definition =="
rg -n -C3 'class QaSettingsRequest|data class QaSettingsRequest|val settings' ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaSettingsRequest.kt
echo
echo "== Other request DTO patterns in same package =="
rg -n -C2 'class .*Request|data class .*Request|val .*: .*=' ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa -g '*.kt'
echo
echo "== Jackson/ObjectMapper custom visibility or mutator config =="
rg -n -C3 'ObjectMapper|Jackson2ObjectMapperBuilder|setVisibility|PropertyAccessor.FIELD|ALLOW_FINAL_FIELDS_AS_MUTATORS|MapperFeature' backend ee -g '*.kt'Repository: tolgee/tolgee-platform
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== QaSettingsRequest usage in endpoints ==="
rg -n 'QaSettingsRequest' backend ee -g '*.kt' -A2 -B2 | head -100
echo
echo "=== Check if QaLanguageSettingsRequest has the same issue ==="
cat ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaLanguageSettingsRequest.kt
echo
echo "=== Look for test cases with deserialization ==="
rg -n 'QaSettingsRequest|QaLanguageSettingsRequest' backend/tests ee/backend/tests -g '*.kt' | head -50Repository: tolgee/tolgee-platform
Length of output: 2557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== QaSettingsController.updateSettings implementation ==="
rg -n -A10 'fun updateSettings' ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.kt
echo
echo "=== Search for tests of QaSettingsController ==="
fd -g '*QaSettings*Test*' . backend ee 2>/dev/null || true
echo
echo "=== Look for any test files mentioning updateSettings ==="
rg -l 'updateSettings.*QaSettings' . --type kt 2>/dev/null || true
echo
echo "=== Check Jackson configuration in Spring config ==="
rg -n 'ObjectMapper|Jackson2ObjectMapperBuilder|@Bean.*ObjectMapper' backend/app/src/main/kotlin/io/tolgee -g '*.kt' | head -20Repository: tolgee/tolgee-platform
Length of output: 2929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List test directories ==="
find . -type d -name "test*" -o -type d -name "*Test*" 2>/dev/null | grep -E '(test|Test)' | head -20
echo
echo "=== Search for any tests in ee/backend ==="
find ee/backend -type d -name "test" -o -type d -name "tests" 2>/dev/null
echo
echo "=== Search for QaSettingsController tests more broadly ==="
rg -l 'QaSettingsController' . -g '*.kt' 2>/dev/null || echo "No tests found"
echo
echo "=== Check for Jackson mixins or custom modules ==="
rg -n 'JacksonModule|Mixin|@JsonCreator|DeserializationConfig' backend/app/src/main/kotlin/io/tolgee -g '*.kt' | head -20
echo
echo "=== Check Spring Boot autoconfiguration for Jackson ==="
rg -n 'spring.jackson|@EnableWebMvc|@Configuration.*Jackson' backend/app -g '*.properties' -g '*.yml' -g '*.kt' | head -30Repository: tolgee/tolgee-platform
Length of output: 915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for QA-related test files in ee/backend/tests ==="
find ee/backend/tests -name '*Qa*' -o -name '*qa*' 2>/dev/null | head -20
echo
echo "=== Search for any test that uses QaSettingsRequest or QaLanguageSettingsRequest ==="
rg -l 'QaSettingsRequest\|QaLanguageSettingsRequest\|QaEnabledRequest' ee/backend/tests -g '*.kt' 2>/dev/null || echo "No matches"
echo
echo "=== Search for updateSettings test ==="
rg -l 'updateSettings' ee/backend/tests -g '*.kt' 2>/dev/null | head -20 || echo "No matches"
echo
echo "=== Check the full QaSettingsController for all endpoints ==="
cat ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/qa/QaSettingsController.ktRepository: tolgee/tolgee-platform
Length of output: 8166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify Kotlin data class vs plain class deserialization behavior ==="
echo "Creating test to understand Jackson behavior with plain Kotlin classes..."
cat > /tmp/test_jackson.kt << 'EOF'
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import com.fasterxml.jackson.module.kotlin.readValue
// Plain class with val and default (problematic pattern)
class PlainRequest {
val settings: Map<String, String> = emptyMap()
}
// Data class with val (correct pattern)
data class DataRequest(
val settings: Map<String, String> = emptyMap()
)
fun main() {
val mapper = jacksonObjectMapper()
val jsonPayload = """{"settings": {"key1": "value1", "key2": "value2"}}"""
println("=== Testing Plain Class ===")
try {
val plainResult = mapper.readValue<PlainRequest>(jsonPayload)
println("Plain class result: ${plainResult.settings}")
println("Is empty? ${plainResult.settings.isEmpty()}")
} catch (e: Exception) {
println("Plain class error: ${e.message}")
}
println("\n=== Testing Data Class ===")
try {
val dataResult = mapper.readValue<DataRequest>(jsonPayload)
println("Data class result: ${dataResult.settings}")
println("Is empty? ${dataResult.settings.isEmpty()}")
} catch (e: Exception) {
println("Data class error: ${e.message}")
}
}
EOF
# Try to run if environment supports it
if command -v kotlinc &> /dev/null; then
kotlinc -script /tmp/test_jackson.kt 2>&1 || echo "Kotlin script execution not available in sandbox"
else
echo "Kotlin not available in sandbox, but Kotlin/Jackson behavior is well-documented:"
echo "- Plain class with val + default: Jackson CANNOT set immutable properties"
echo "- Data class: Jackson uses generated constructor to set all properties"
fi
echo
echo "=== Checking if there's any `@JsonCreator` in the DTOs ==="
cat ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaSettingsRequest.kt
echo
cat ee/backend/app/src/main/kotlin/io/tolgee/ee/data/qa/QaEnabledRequest.ktRepository: tolgee/tolgee-platform
Length of output: 800
Convert to data class to enable proper JSON deserialization of incoming settings payload.
At Line 6-7, QaSettingsRequest is a plain class with an immutable val property and default value. During Spring/Jackson deserialization, incoming JSON payloads are silently ignored because Jackson cannot mutate val properties on plain classes—the settings field will always be emptyMap() regardless of the request body.
The same pattern appears in QaLanguageSettingsRequest and QaEnabledRequest in the same package and will cause identical deserialization failures.
Proposed fix
-class QaSettingsRequest {
- val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap()
-}
+data class QaSettingsRequest(
+ val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap(),
+)Apply the same fix to QaLanguageSettingsRequest and QaEnabledRequest.
📝 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.
| class QaSettingsRequest { | |
| val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap() | |
| data class QaSettingsRequest( | |
| val settings: Map<QaCheckType, QaCheckSeverity?> = emptyMap(), | |
| ) |
🤖 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/data/qa/QaSettingsRequest.kt`
around lines 6 - 7, QaSettingsRequest is a plain class with an immutable val
initialized to emptyMap(), so Jackson can't populate incoming JSON; change it to
a Kotlin data class with a constructor parameter (e.g. data class
QaSettingsRequest(val settings: Map<QaCheckType, QaCheckSeverity?> =
emptyMap())) so Jackson can deserialize into it, and apply the same change
pattern to QaLanguageSettingsRequest and QaEnabledRequest (convert each to a
data class with constructor properties and sensible defaults).
| val langConfig = | ||
| languageQaConfigRepository.findByLanguageProjectIdAndLanguageId(projectId, languageId) | ||
| ?: LanguageQaConfig( | ||
| language = entityManager.getReference(Language::class.java, languageId), | ||
| ) |
There was a problem hiding this comment.
Scope new LanguageQaConfig records to projectId.
The create path builds a config from a bare languageId reference and never verifies that the language belongs to the requested project. That lets a project-scoped call persist overrides onto another project's language if the caller knows the ID.
🤖 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/qa/ProjectQaConfigService.kt`
around lines 125 - 129, The code creates a new LanguageQaConfig using only a
bare language reference which can allow cross-project writes; update the create
path in ProjectQaConfigService so you first resolve and validate that the
Language with id languageId belongs to the requested projectId (e.g. load the
Language entity or its LanguageProject relation and assert its project.id ==
projectId), and then construct LanguageQaConfig(language = ..., project = ...)
(or set the project reference on the new LanguageQaConfig) before saving; locate
the logic around
languageQaConfigRepository.findByLanguageProjectIdAndLanguageId,
entityManager.getReference(Language::class.java, languageId), LanguageQaConfig,
projectId and languageId to implement the validation and to scope the new config
to the project.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@webapp/src/views/projects/translations/context/services/useWebsocketService.ts`:
- Around line 95-115: The effect subscribing to
`/projects/${project.id}/qa-checks-completed` uses
translationService.changeTranslations but does not list translationService in
the dependency array, causing a stale closure; update the useEffect dependencies
to include translationService (the object returned by useTranslationsService) so
the effect re-runs when translationService changes and continues to call
changeTranslations on the current service instance; keep the existing
client.subscribe return cleanup logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6028bee9-a27b-4f6b-b922-22a6a06b9509
📒 Files selected for processing (1)
webapp/src/views/projects/translations/context/services/useWebsocketService.ts
| useEffect(() => { | ||
| if (client && qaChecksEnabled) { | ||
| return client.subscribe( | ||
| `/projects/${project.id}/qa-checks-completed`, | ||
| (event) => { | ||
| translationService.changeTranslations([ | ||
| { | ||
| keyId: event.data.keyId, | ||
| language: event.data.languageTag, | ||
| value: { | ||
| id: event.data.translationId, | ||
| qaIssueCount: event.data.qaIssueCount, | ||
| qaChecksStale: event.data.qaChecksStale, | ||
| qaIssues: event.data.qaIssues, | ||
| }, | ||
| }, | ||
| ]); | ||
| } | ||
| ); | ||
| } | ||
| }, [project, client, qaChecksEnabled]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate useTranslationsService implementation and check memoization =="
fd '^useTranslationsService\.ts$' webapp/src -t f -x sh -c '
echo "### FILE: $1"
rg -n -C3 "export const useTranslationsService|return \\{|useMemo\\(|useCallback\\(" "$1"
' sh {}
echo
echo "== Locate useWebsocketService call sites =="
rg -n -C2 '\buseWebsocketService\s*\(' webapp/src
echo
echo "== Check React hooks exhaustive-deps linting presence =="
rg -n -C2 'react-hooks/exhaustive-deps|eslint-plugin-react-hooks' webappRepository: tolgee/tolgee-platform
Length of output: 1490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read useWebsocketService.ts full content ==="
cat -n webapp/src/views/projects/translations/context/services/useWebsocketService.ts | head -150
echo
echo "=== Check TranslationsContext.ts around line 142 to see translationService source ==="
cat -n webapp/src/views/projects/translations/context/TranslationsContext.ts | sed -n '130,160p'
echo
echo "=== Search for translationService definition in TranslationsContext ==="
rg -n "translationService\s*=" webapp/src/views/projects/translations/context/TranslationsContext.ts | head -20Repository: tolgee/tolgee-platform
Length of output: 6092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find useTranslationsService.ts ==="
fd 'useTranslationsService\.ts$' webapp/src -t f
echo
echo "=== Read useTranslationsService implementation ==="
cat -n "$(fd 'useTranslationsService\.ts$' webapp/src -t f | head -1)"Repository: tolgee/tolgee-platform
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List files in services directory ==="
ls -la webapp/src/views/projects/translations/context/services/
echo
echo "=== Search for useTranslationsService definition ==="
rg -n "useTranslationsService" webapp/src/views/projects/translations/context/services/
echo
echo "=== Broader search for useTranslationsService definition ==="
rg -n "export.*useTranslationsService" webapp/srcRepository: tolgee/tolgee-platform
Length of output: 4520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read useTranslationsService.tsx - focus on return and useMemo ==="
cat -n webapp/src/views/projects/translations/context/services/useTranslationsService.tsx | grep -A 5 -B 5 "return {"Repository: tolgee/tolgee-platform
Length of output: 2166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read useTranslationsService.tsx return statement (lines 399 onwards) ==="
cat -n webapp/src/views/projects/translations/context/services/useTranslationsService.tsx | sed -n '399,450p'Repository: tolgee/tolgee-platform
Length of output: 1302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check how useTranslationsService is called in TranslationsContext ==="
cat -n webapp/src/views/projects/translations/context/TranslationsContext.ts | sed -n '115,135p'
echo
echo "=== Check if useTranslationsService return is wrapped with useMemo anywhere ==="
rg -n "useMemo.*useTranslationsService" webapp/src
echo
echo "=== Check for useMemo wrapping in TranslationsContext ==="
rg -n "useMemo" webapp/src/views/projects/translations/context/TranslationsContext.ts | head -10
echo
echo "=== Check the beginning of TranslationsContext component ==="
cat -n webapp/src/views/projects/translations/context/TranslationsContext.ts | sed -n '1,50p'Repository: tolgee/tolgee-platform
Length of output: 1083
🏁 Script executed:
#!/bin/bash
set -euoelpipefail
echo "=== Check WebsocketClient subscribe method to understand if callback captures are an issue ==="
rg -n "subscribe\(" webapp/src -t ts -t tsx -B2 -A5 | head -50
echo
echo "=== Check if changeTranslations mutates state or returns promises ==="
rg -n "changeTranslations" webapp/src/views/projects/translations/context/services/useTranslationsService.tsx -B3 -A10 | head -40Repository: tolgee/tolgee-platform
Length of output: 740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if changeTranslations mutates state or returns promises ==="
rg -n "const changeTranslations|changeTranslations\s*=" webapp/src/views/projects/translations/context/services/useTranslationsService.tsx -B3 -A15
echo
echo "=== Check WebsocketClient implementation to understand callback behavior ==="
find webapp/src -name "*WebsocketClient*" -type f | head -5Repository: tolgee/tolgee-platform
Length of output: 878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read WebsocketClient.ts subscribe method ==="
cat -n webapp/src/websocket-client/WebsocketClient.ts | head -100
echo
echo "=== Check subscribe method signature and implementation ==="
rg -n "subscribe\s*\(" webapp/src/websocket-client/WebsocketClient.ts -A10 | head -50Repository: tolgee/tolgee-platform
Length of output: 4060
Add translationService dependency to the QA effect.
Line 100 uses translationService.changeTranslations(), but the dependency array at line 115 is missing translationService. Since useTranslationsService() in TranslationsContext returns a new object on each render (with no memoization), the effect closure will reference a stale translationService when it doesn't re-run after translationService changes.
Suggested fix
- }, [project, client, qaChecksEnabled]);
+ }, [project, client, qaChecksEnabled, translationService]);📝 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.
| useEffect(() => { | |
| if (client && qaChecksEnabled) { | |
| return client.subscribe( | |
| `/projects/${project.id}/qa-checks-completed`, | |
| (event) => { | |
| translationService.changeTranslations([ | |
| { | |
| keyId: event.data.keyId, | |
| language: event.data.languageTag, | |
| value: { | |
| id: event.data.translationId, | |
| qaIssueCount: event.data.qaIssueCount, | |
| qaChecksStale: event.data.qaChecksStale, | |
| qaIssues: event.data.qaIssues, | |
| }, | |
| }, | |
| ]); | |
| } | |
| ); | |
| } | |
| }, [project, client, qaChecksEnabled]); | |
| useEffect(() => { | |
| if (client && qaChecksEnabled) { | |
| return client.subscribe( | |
| `/projects/${project.id}/qa-checks-completed`, | |
| (event) => { | |
| translationService.changeTranslations([ | |
| { | |
| keyId: event.data.keyId, | |
| language: event.data.languageTag, | |
| value: { | |
| id: event.data.translationId, | |
| qaIssueCount: event.data.qaIssueCount, | |
| qaChecksStale: event.data.qaChecksStale, | |
| qaIssues: event.data.qaIssues, | |
| }, | |
| }, | |
| ]); | |
| } | |
| ); | |
| } | |
| }, [project, client, qaChecksEnabled, translationService]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@webapp/src/views/projects/translations/context/services/useWebsocketService.ts`
around lines 95 - 115, The effect subscribing to
`/projects/${project.id}/qa-checks-completed` uses
translationService.changeTranslations but does not list translationService in
the dependency array, causing a stale closure; update the useEffect dependencies
to include translationService (the object returned by useTranslationsService) so
the effect re-runs when translationService changes and continues to call
changeTranslations on the current service instance; keep the existing
client.subscribe return cleanup logic intact.
Add the QA checks feature framework with an "empty translation" check
running from the backend live preview endpoint through to the QA panel
in the translation tools sidebar.
Backend:
- QaCheckType, QaCheckSeverity, QaIssueState, QaIssueMessage enums
- TranslationQaIssue entity with Liquibase migration
- Pluggable QaCheck interface with EmptyTranslationCheck implementation
- QaCheckRunnerService orchestrator
- POST /v2/projects/{projectId}/qa-check/preview endpoint (EE, gated)
- QaCheckPreviewControllerTest (3 tests)
Frontend:
- useQaCheckPreview hook (follows glossary pattern)
- QaChecksPanel and QaCheckItem components
- Registered in eeModule.ee.tsx via addPanel pattern
- API schema regenerated with new endpoint
Switch @Enumerated to EnumType.STRING for type, message, and state columns to avoid fragile ordinal-based storage.
Wrap each check in try-catch so a failing check produces a fallback issue (with QA_CHECK_FAILED message) instead of breaking the entire run.
…for glossary panel
Add 4 QA check implementations that compare translation text against the base language, giving the QA panel real substance beyond the initial empty-translation check. Backend: - Add params field to QaCheckResult pipeline for parameterized messages - Add 14 QaIssueMessage enum values - Update preview controller to fetch base translation text from DB - Implement SpacesMismatchCheck (7 variants), PunctuationMismatchCheck (3 variants), CharacterCaseMismatchCheck (2 variants), and MissingNumbersCheck Frontend: - Extend useQaIssueMessage to accept params and handle 14 new cases - Add 4 check type labels to useQaCheckTypeLabel - Add params field to QaCheckResultItem type - Pass params through QaCheckItem to message hook Tests: - 37 unit tests across 4 check test classes - 7 integration tests in QaCheckPreviewControllerTest
…rom base language
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.kt (1)
49-67:⚠️ Potential issue | 🟠 MajorUse full issue identity (
replacement+ serializedparams) for state carry-over and param-based lookup.Line 52-58 and Line 178-187 still match issues without
params, and Line 52-58 also omitsreplacement. This can carry ignored/open state to the wrong variant and make ignore/unignore inconsistent across similar checks.Suggested fix
@@ val entities = results.map { result -> + val serializedParams = result.params?.let { objectMapper.writeValueAsString(it) } val matchingExisting = existingIssues.find { existing -> existing.type == result.type && existing.message == result.message && + existing.replacement == result.replacement && existing.positionStart == result.positionStart && existing.positionEnd == result.positionEnd && - existing.pluralVariant == result.pluralVariant + existing.pluralVariant == result.pluralVariant && + existing.params == serializedParams } TranslationQaIssue( @@ - params = result.params?.let { objectMapper.writeValueAsString(it) }, + params = serializedParams, @@ private fun findMatchingIssue( @@ return qaIssueRepository.findByProjectIdAndTranslationIdAndIssueParams( projectId, translationId, request.type, request.message, request.replacement, request.positionStart, request.positionEnd, request.pluralVariant, + request.params?.let { objectMapper.writeValueAsString(it) }, ) }Also applies to: 173-187
🤖 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/qa/QaIssueService.kt` around lines 49 - 67, The current matching logic in existingIssues.find and the state carry-over when constructing TranslationQaIssue omits `replacement` and uses raw `params`, which causes state to be shared incorrectly; update the comparison used in existingIssues.find (and any param-based lookups around the same code paths) to include both `replacement` and a serialized `params` string (use objectMapper.writeValueAsString(result.params) to normalize null/empty vs actual params) and use that same serialized params + replacement when deciding state (matchingExisting?.state) so ignore/open state is preserved per exact issue identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.kt`:
- Around line 147-148: The code currently sets isVirtual = issue == null which
marks any existing issue as non-virtual; instead derive the flag from the issue
when present. Change the isVirtual computation used before
publishQaIssueStateChange("QA_ISSUE_IGNORED", projectId, request.type,
isVirtual) to use the issue's virtual property (e.g. isVirtual =
issue?.isVirtual ?: true) so existing virtual issues are reported correctly, and
pass that corrected isVirtual into publishQaIssueStateChange.
In
`@webapp/src/views/projects/translations/context/services/useWebsocketService.ts`:
- Around line 35-51: The keyUpdates mapping currently sets value to { deleted:
true } for deletions but does not clear the deleted flag on restores; update the
mapping (the keyUpdates computation that uses event.data?.keys?.map and
getModifyingObject) so the non-deleted branch explicitly includes deleted: false
before spreading the modifying object (so when the reducer merges updates with {
...k, ...value } the deleted flag is cleared on restore).
---
Duplicate comments:
In `@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.kt`:
- Around line 49-67: The current matching logic in existingIssues.find and the
state carry-over when constructing TranslationQaIssue omits `replacement` and
uses raw `params`, which causes state to be shared incorrectly; update the
comparison used in existingIssues.find (and any param-based lookups around the
same code paths) to include both `replacement` and a serialized `params` string
(use objectMapper.writeValueAsString(result.params) to normalize null/empty vs
actual params) and use that same serialized params + replacement when deciding
state (matchingExisting?.state) so ignore/open state is preserved per exact
issue identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4a4c25-17c0-4885-b281-cc1de07fd826
📒 Files selected for processing (4)
backend/data/src/main/kotlin/io/tolgee/websocket/WebsocketEventType.ktee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.ktwebapp/src/views/projects/translations/context/services/useWebsocketService.tswebapp/src/websocket-client/WebsocketClient.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/data/src/main/kotlin/io/tolgee/websocket/WebsocketEventType.kt
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/QaIssueService.kt
Outdated
Show resolved
Hide resolved
| const keyUpdates = event.data?.keys?.map((key) => { | ||
| const isDeleted = | ||
| key.changeType === 'DEL' || | ||
| (key.modifications as Record<string, any>)?.deletedAt?.new != null | ||
| (key.modifications as Record<string, Modification<unknown>>)?.deletedAt | ||
| ?.new != null; | ||
|
|
||
| return { | ||
| keyId: key.id, | ||
| value: isDeleted | ||
| ? { deleted: true } | ||
| : { | ||
| ...getModifyingObject(key.modifications, { | ||
| name: 'keyName', | ||
| }), | ||
| }, | ||
| })); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
svc="$(fd '^useTranslationsService\.tsx?$' webapp/src/views/projects/translations/context/services -t f | head -1)"
echo "=== updateTranslationKeys definition ==="
rg -n -C4 '\bupdateTranslationKeys\b' "$svc"
echo
echo "=== deleted / deletedAt handling in translations context ==="
rg -n -C3 '\bdeleted\b|\bdeletedAt\b' webapp/src/views/projects/translations/contextRepository: tolgee/tolgee-platform
Length of output: 5145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full updateTranslationKeys implementation
echo "=== Full updateTranslationKeys implementation ==="
rg -n -A 20 'const updateTranslationKeys = ' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
echo
echo "=== KeyUpdateData type definition ==="
rg -n -B2 -A5 'type KeyUpdateData' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
echo
echo "=== getModifyingObject function ==="
rg -n -B2 -A10 'function getModifyingObject\|const getModifyingObject' webapp/src/views/projects/translations/context/services/useWebsocketService.tsRepository: tolgee/tolgee-platform
Length of output: 838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find KeyUpdateData type definition across all files
echo "=== KeyUpdateData type definition ==="
rg -n 'type KeyUpdateData' webapp/src/views/projects/translations/context/
echo
echo "=== Full useWebsocketService.ts file ==="
cat -n webapp/src/views/projects/translations/context/services/useWebsocketService.tsRepository: tolgee/tolgee-platform
Length of output: 5005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full KeyUpdateData type definition
echo "=== KeyUpdateData type definition ==="
rg -n -A 5 'export type KeyUpdateData' webapp/src/views/projects/translations/context/types.ts
echo
echo "=== Check if deleted field is used elsewhere in the restore logic ==="
rg -n 'deleted.*false' webapp/src/views/projects/translations/context/services/Repository: tolgee/tolgee-platform
Length of output: 366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at ValueUpdate type definition
echo "=== ValueUpdate type definition ==="
rg -n -B2 -A5 'type ValueUpdate' webapp/src/views/projects/translations/context/types.ts
echo
echo "=== How deleted field is used in the UI ==="
rg -n '\bdele\(ted\|tedAt\)' webapp/src/views/projects/translations/context/ | head -30
echo
echo "=== Check if there's any post-processing that syncs deleted with deletedAt ==="
rg -n 'deleted.*deletedAt\|deletedAt.*deleted' webapp/src/views/projects/translations/context/services/useTranslationsService.tsxRepository: tolgee/tolgee-platform
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what the reducer does with the value spread
echo "=== How the key reducer applies updates (full reducer context) ==="
sed -n '320,340p' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
echo
echo "=== Search for deleted field usage in rendering/filtering ==="
rg -n '\bdeleted\b' webapp/src/views/projects/translations --type tsx | grep -E 'filter|show|hide|condition|?.deleted|!.*deleted'
echo
echo "=== Check initial data loading - how keys are created with deleted flag ==="
rg -n -B3 -A3 'deleted.*true|deleted:.*true' webapp/src/views/projects/translations/context/services/useTranslationsService.tsxRepository: tolgee/tolgee-platform
Length of output: 880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check usages of the deleted property more carefully
echo "=== All references to '.deleted' ==="
rg -n '\.deleted' webapp/src/views/projects/translations/context/
echo
echo "=== Check if there's any computed/derived deleted flag ==="
rg -n 'deleted\s*=' webapp/src/views/projects/translations/context/ | grep -v 'deletedAt'
echo
echo "=== Check the initial state or data transformation when keys are loaded ==="
rg -n -A10 'const getTranslations' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx | head -40Repository: tolgee/tolgee-platform
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search broadly for how deletion state is tracked
echo "=== Check what fields KeyWithTranslationsModel actually has ==="
fd -t f schema.json webapp/src/views/projects/translations/ | xargs cat 2>/dev/null | jq -r '.components.schemas.KeyWithTranslationsModel.properties | keys[] | select(contains("delete"))'
echo
echo "=== Search for how keys are filtered/displayed ==="
rg -n 'fixedTranslations.*filter|\.filter.*deleted|show.*deleted' webapp/src/views/projects/translations/
echo
echo "=== Check if there's any logic that checks deletedAt vs deleted ==="
rg -n 'deletedAt' webapp/src/views/projects/translations/ -l
echo
echo "=== Look at how keys are displayed or filtered in the component layer ==="
rg -n 'DeletableKeyWithTranslationsModelType' webapp/src/views/projects/translations/ -A3 | head -40Repository: tolgee/tolgee-platform
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the types.ts file more carefully to understand the relationship
echo "=== Full context around DeletableKeyWithTranslationsModelType ==="
rg -n -B5 -A10 'export type DeletableKeyWithTranslationsModelType' webapp/src/views/projects/translations/context/types.ts
echo
echo "=== Search in components or hooks for where deleted flag is actually used ==="
rg -n '\bdeleted\b' webapp/src/views/projects/translations/components/ webapp/src/views/projects/translations/hooks/ 2>/dev/null | head -20
echo
echo "=== Check test files or storybook for usage patterns ==="
find webapp/src/views/projects/translations -name '*.test.*' -o -name '*.stories.*' | xargs rg -l 'deleted' 2>/dev/nullRepository: tolgee/tolgee-platform
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's search more broadly for deleted usage
echo "=== All files that mention 'deleted' in translations context ==="
rg -l 'deleted' webapp/src/views/projects/translations/
echo
echo "=== Show specific usages of deleted in those files ==="
rg -n 'deleted' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
echo
echo "=== Look at actual component usage - how keys are rendered ==="
rg -n '\bdeleted\b' webapp/src/views/projects/translations/components/ 2>/dev/null
echo
echo "=== Check if the deleted flag affects any filtering or visibility logic ==="
rg -n '\bdeleted\b' webapp/src/views/projects/translations/hooks/ 2>/dev/nullRepository: tolgee/tolgee-platform
Length of output: 1049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Now let's see how deleted is used in trash and filter components
echo "=== TrashTrashedCell.tsx - how deleted flag is used ==="
rg -n -C3 'deleted' webapp/src/views/projects/translations/trash/TrashTrashedCell.tsx
echo
echo "=== TrashPage.tsx ==="
rg -n -C3 'deleted' webapp/src/views/projects/translations/trash/TrashPage.tsx
echo
echo "=== RowTable.tsx ==="
rg -n -C3 'deleted' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx
echo
echo "=== RowList.tsx ==="
rg -n -C3 'deleted' webapp/src/views/projects/translations/TranslationsList/RowList.tsx
echo
echo "=== SubfilterDeletedBy.tsx ==="
rg -n -C3 'deleted' webapp/src/views/projects/translations/TranslationFilters/SubfilterDeletedBy.tsxRepository: tolgee/tolgee-platform
Length of output: 3668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any derived/computed deleted flag from deletedAt
echo "=== Look for any getters or computed properties that derive deleted from deletedAt ==="
rg -n 'get deleted|deleted.*deletedAt|deletedAt.*deleted' webapp/src/views/projects/translations/context/
echo
echo "=== Check how data flows from initial load into fixedTranslations ==="
rg -n -B3 -A3 'setFixedTranslations.*data\|data.*fixedTranslations' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx | head -50
echo
echo "=== Check if deleted is ever explicitly set to false ==="
rg -n 'deleted.*false' webapp/src/views/projects/translations/context/services/useWebsocketService.tsRepository: tolgee/tolgee-platform
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact reducer logic once more - does spreading handle undefined/null removal?
echo "=== Verify reducer spread behavior - how keys are updated ==="
sed -n '323,332p' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx
echo
echo "=== Check what getModifyingObject actually returns on restore (deletedAt: null) ==="
sed -n '126,135p' webapp/src/views/projects/translations/context/services/useWebsocketService.ts
echo
echo "=== Look at the actual rendering - is deleted flag ever cleared anywhere? ==="
rg -n 'deleted\s*:\s*false|deleted\s*=\s*false' webapp/src/views/projects/translations/
echo
echo "=== Check if there's a transformation when fetching fresh data ==="
rg -n -A5 'useQuery\|usePaginatedQuery' webapp/src/views/projects/translations/context/services/useTranslationsService.tsx | head -50Repository: tolgee/tolgee-platform
Length of output: 918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if RowTable/RowList ever check deletedAt instead of deleted
echo "=== How RowTable determines the deleted class ==="
rg -n -B5 -A5 'className.*deleted' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx
echo
echo "=== Check the data object type in those components ==="
rg -n 'interface.*Row|type.*Row' webapp/src/views/projects/translations/TranslationsTable/ webapp/src/views/projects/translations/TranslationsList/
echo
echo "=== Search for any computed property that derives deleted from deletedAt ==="
ast-grep --pattern 'const $_deleted = $$$deletedAt$$$'
echo
echo "=== Check if DeletableKeyWithTranslationsModelType has any getters or computed fields ==="
rg -n -A20 'KeyWithTranslationsModel' webapp/src/views/projects/translations/context/types.ts | grep -A15 'DeletableKeyWithTranslationsModelType'Repository: tolgee/tolgee-platform
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the data prop in RowTable has type definition
echo "=== RowTable component props type ==="
rg -n -B10 'interface.*Props\|type.*Props' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx | head -30
echo
echo "=== What is the 'data' prop type ==="
rg -n 'data:.*\|data as ' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx | head -10
echo
echo "=== Check if deleted is ever computed or derived from deletedAt ==="
rg -n -B2 -A2 'Object.defineProperty\|get\s\w*deleted\|deleted\s*:\s*function' webapp/src/views/projects/translations/context/Repository: tolgee/tolgee-platform
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read RowTable.tsx to see the component structure
echo "=== RowTable.tsx component definition ==="
head -100 webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx | tail -50
echo
echo "=== Search for where data is being used as a variable ==="
rg -n 'function RowTable\|const RowTable\|export.*RowTable' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx
echo
echo "=== Full first 80 lines of RowTable ==="
head -80 webapp/src/views/projects/translations/TranslationsTable/RowTable.tsxRepository: tolgee/tolgee-platform
Length of output: 1704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full function signature of RowTable
echo "=== RowTable function signature and imports ==="
head -50 webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx
echo
echo "=== Look for the export and function declaration ==="
rg -n 'export.*RowTable\|function RowTable\|const RowTable' webapp/src/views/projects/translations/TranslationsTable/RowTable.tsx -A 5Repository: tolgee/tolgee-platform
Length of output: 1520
On key restore, explicitly set deleted: false.
When a key is deleted, the code sends { deleted: true } (line 44). However, on restore (when deletedAt changes from non-null to null), the else branch only forwards the modifications via getModifyingObject, which would return { deletedAt: null }. Since the reducer spreads updates directly ({ ...k, ...value }), this leaves the deleted field untouched—the key remains marked deleted even though it's been restored. The UI will incorrectly show strikethrough styling and disabled interaction.
Change line 43-49 to explicitly clear the flag on restore:
value: isDeleted
? { deleted: true }
: {
deleted: false,
...getModifyingObject(key.modifications, {
name: 'keyName',
}),
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@webapp/src/views/projects/translations/context/services/useWebsocketService.ts`
around lines 35 - 51, The keyUpdates mapping currently sets value to { deleted:
true } for deletions but does not clear the deleted flag on restores; update the
mapping (the keyUpdates computation that uses event.data?.keys?.map and
getModifyingObject) so the non-deleted branch explicitly includes deleted: false
before spreading the modifying object (so when the reducer merges updates with {
...k, ...value } the deleted flag is cleared on restore).
- Change @TransactionalEventListener phase to BEFORE_COMMIT for QA listeners to ensure consistency. - Remove redundant try-catch blocks and logging from event handlers. - Optimize sibling target computation for QA checks after base text changes. - Batch save QA-stale translations to reduce queries.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaCheckTranslationListener.kt (1)
59-64: Consider removing redundantdistinctBy.If
key.translationsis mapped as a JPASetorMutableSet, there cannot be duplicates, making.distinctBy { it.id }unnecessary. If it's aListmapping, this is valid defensive code.#!/bin/bash # Check the type of the translations collection in Key entity ast-grep --pattern $'class Key { $$$ translations$_ $$$ }' # Fallback: search for translations property definition in Key rg -n -A3 'translations.*:.*Translation' --type=kotlin🤖 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/component/qa/QaCheckTranslationListener.kt` around lines 59 - 64, The use of .distinctBy { it.id } on event.key.translations is likely redundant if Key.translations is a Set/MutableSet; remove the distinctBy call to avoid unnecessary allocations and simplify the code: change the pipeline that builds existingSiblings to filter out base/excluded languages directly from event.key.translations (keep the .filter { it.language.id != baseLanguageId && it.language.id !in excludedLanguageIds }), then mark each existingSiblings.forEach { it.qaChecksStale = true } and call translationService.saveAll(existingSiblings) as before; verify Key.translations is indeed a Set/MutableSet (if it is a List and duplicates are possible, keep distinctBy).ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaFeatureAccessListener.kt (1)
44-51: Prefer bulk disable/retry over per-project transactional loop.Line 44 iterates project-by-project and calls
setQaEnabledper item; for large org/global changes this can become slow and partially complete without structured retry. Consider a bulk service path (batched IDs + failure summary/retry queue).♻️ Suggested direction
- projects.forEach { project -> - try { - projectQaConfigService.setQaEnabled(project.id, false) - } catch (e: Exception) { - logger.error("Failed to disable QA checks for project ${project.id}", e) - Sentry.captureException(e) - } - } + val projectIds = projects.map { it.id } + val failedIds = projectQaConfigService.disableQaInBulk(projectIds) + if (failedIds.isNotEmpty()) { + logger.warn("QA disable partially failed for projects: {}", failedIds) + }🤖 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/component/qa/QaFeatureAccessListener.kt` around lines 44 - 51, The current per-project loop in QaFeatureAccessListener (projects.forEach { project -> projectQaConfigService.setQaEnabled(project.id, false) }) should be replaced with a bulk-disable path: add a new service method on ProjectQaConfigService (e.g., disableQaForProjects(projectIds: List<Long>)) that updates project QA flags in batches/within a single transaction (or appropriate chunked transactions), returns a summary of successes/failures, and enqueues failed IDs for retry (or exposes them for a retry queue); update QaFeatureAccessListener to call this bulk method with the collected project IDs, log a concise failure summary instead of per-item exceptions, and capture/report exceptions from the bulk operation (including adding retry metadata) so large org/global changes are fast, atomic per-batch, and retriable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaCheckTranslationListener.kt`:
- Around line 59-64: The use of .distinctBy { it.id } on event.key.translations
is likely redundant if Key.translations is a Set/MutableSet; remove the
distinctBy call to avoid unnecessary allocations and simplify the code: change
the pipeline that builds existingSiblings to filter out base/excluded languages
directly from event.key.translations (keep the .filter { it.language.id !=
baseLanguageId && it.language.id !in excludedLanguageIds }), then mark each
existingSiblings.forEach { it.qaChecksStale = true } and call
translationService.saveAll(existingSiblings) as before; verify Key.translations
is indeed a Set/MutableSet (if it is a List and duplicates are possible, keep
distinctBy).
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaFeatureAccessListener.kt`:
- Around line 44-51: The current per-project loop in QaFeatureAccessListener
(projects.forEach { project -> projectQaConfigService.setQaEnabled(project.id,
false) }) should be replaced with a bulk-disable path: add a new service method
on ProjectQaConfigService (e.g., disableQaForProjects(projectIds: List<Long>))
that updates project QA flags in batches/within a single transaction (or
appropriate chunked transactions), returns a summary of successes/failures, and
enqueues failed IDs for retry (or exposes them for a retry queue); update
QaFeatureAccessListener to call this bulk method with the collected project IDs,
log a concise failure summary instead of per-item exceptions, and capture/report
exceptions from the bulk operation (including adding retry metadata) so large
org/global changes are fast, atomic per-batch, and retriable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7df425c8-565f-4c74-988f-0685a9b9431a
📒 Files selected for processing (4)
ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaBaseLanguageChangeListener.ktee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaCheckTranslationListener.ktee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaFeatureAccessListener.ktee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaLanguageTagChangeListener.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/backend/app/src/main/kotlin/io/tolgee/ee/component/qa/QaBaseLanguageChangeListener.kt
|
|
||
| @Service | ||
| class LanguageToolService { | ||
| private val threadLocalInstances: ThreadLocal<MutableMap<String, JLanguageTool>> = |
There was a problem hiding this comment.
This is pure AI finding, I suppose it is worth to elaborate it more.
JLanguageTool instances are heavyweight (50–100 MB each for languages with spelling rules). Using an unbounded ThreadLocal means each thread in the servlet pool accumulates instances for every language it serves, and they are never evicted.
Risk scenario: With a Tomcat thread pool of 200 threads and 30+ supported languages, the worst case is 200 × 30 = 6,000 JLanguageTool instances, consuming 300+ GB of heap. In practice, traffic distribution means this won't hit the theoretical maximum, but even a fraction causes serious GC pressure and potential OOM crashes. The result cache (Caffeine, 200 entries, 5 min TTL) mitigates instance usage but not instance creation.
There was a problem hiding this comment.
Good point. I'll be switching to external instance of the language tool, so hopefully it won't be issue there.
| ): List<QaCheckResult> { | ||
| val enabledTypes = | ||
| if (languageId != null) { | ||
| projectQaConfigService.getEnabledCheckTypesForLanguage(projectId, languageId) |
There was a problem hiding this comment.
I’m concerned that the usage in the batch processor could lead to a significant number of queries, just to retrieve the configuration. Additionally, if there are multiple combinations of the same project and languageId, the same query might be repeated.
Maybe if you could prefetch the config ?
| } | ||
|
|
||
| @Transactional | ||
| fun setQaChecksStale(translationIds: List<Long>) { |
There was a problem hiding this comment.
This is called in multiple places, and translationIds are loaded solely for this update — I'm not sure how many translation IDs there typically are, but it might be worth considering a single bulk edit instead.
There was a problem hiding this comment.
Hmm, I'm not sure I understand. This function exists for this purpose - to set the stale flag in a single bulk query.
Previously, I used to identify for which translations we need to run QA by their IDs, so this query is still using translation IDs. I thought about migrating it to keyIDs+languageIDs like batch jobs already use, but it didn't seem to be worth it.
There was a problem hiding this comment.
I wanted to point out that to update the stale flag, you first need to retrieve all the translation IDs. Yhis can be done by using a potentially heavy SELECT query and then perform an UPDATE operation, passing these translationIds.
My question is, is it possible to replace this with a single UPDATE operation ?
There was a problem hiding this comment.
Thanks for the clarification ^^
That might be possible. I still need to pull a lot of data to start the batch job, but that shouldn't be much different then what already existing batch operations do when starting up batch translate for the whole project. I'll check if it's possible to make a specialized version of this for situations when I don't have/need translationIds.
The resolveLanguage method was unconditionally following defaultLanguageVariant, which redirected regional variants to the "canonical" variant (e.g., en-GB → en-US, pt-BR → pt-PT). This caused users with British English or Brazilian Portuguese content to get spelling results for the wrong region. Now only base languages (no country code) follow defaultLanguageVariant. Regional variants are returned as-is. Also fixes cache key and ThreadLocal instance key to use shortCodeWithCountryAndVariant so different variants get separate entries. Removes dead variant-scan fallback branch and adds comprehensive unit tests for resolveLanguage covering all resolution paths.
LanguageStatsListener only handled OnBatchJobSucceeded for QA_CHECK batch jobs, leaving cached qaChecksStaleCount stale when jobs failed or were cancelled. Since QA_CHECK is a hidden batch job (no activityType), the onActivity listener couldn't cover this gap. Add OnBatchJobFailed and OnBatchJobCancelled handlers following the BatchJobActivityFinalizer three-handler pattern, extracting shared logic into onQaBatchJobCompleted().
Security: validate translationId ownership in ignore/unignore endpoints. Data integrity: include replacement in state-preservation match, fix MissingNumbersCheck to use multiset comparison, only clear qaChecksStale on full rechecks. Fix duplicate unique constraints in QA config entities. Add missing @ReadOnlyOperation annotations. Don't leak exception messages in WebSocket error handler. Remove redundant save in batch service. Frontend: fix dead null-check in ProjectSettingsQa, fix i18n concatenation in IssueRow, fix WebSocket loading state, add missing useEffect dependency, use stable keys, remove unnecessary type casts, add QA-specific translation keys, route QaIssueHighlight through EE module with shared props type.
QA entities (TranslationQaIssue, ProjectQaConfig, LanguageQaConfig) lacked application-level cascade deletion, causing FK constraint violations when deleting projects, languages, or branches. Fix 5 broken deletion paths: - TranslationService.deleteAllByProject: clean QA issues before translations - LanguageService.deleteAllByProject: clean language QA configs before languages - ProjectHardDeletingService: clean project QA config before project - LanguageHardDeleter: clean language QA config before individual language - BranchCleanupService: clean QA issues before branch translations
…vents, and dashboard stats Fill testing gaps in QA checks feature: - QaSettingsControllerTest: settings CRUD, enable/disable toggle, per-language overrides - QaTranslationFilterTest: filter by QA issues and check types - QaStaleFlowTest: stale flag lifecycle from save through batch completion - QaEventListenerTest: feature access, project creation, base language and tag change listeners - QaDashboardStatsTest: language stats QA issue and stale counts - qaUtils.test.ts: frontend pure utility function tests Boy Scout improvements: - Extract repeated QaCheckIssueIgnoreRequest into helper in QaIssueControllerTest - Use assertThat directly in QaBatchServiceTest - Fix pre-existing QaTestUtil compilation error (missing translationId param) - Document defensive guard in qaUtils.ts adjustQaIssuesForVariant
Add comprehensive Cypress E2E tests covering all QA checks user flows: - Panel (issues display, empty state, project-disabled state) - Cell badges (count, visibility, ignored-only exclusion) - Inline highlighting (positional, zero-width markers) - Issue actions (ignore, unignore, correct with replacement) - Settings (toggle, severity changes, per-language overrides) - Filtering (any QA issue, specific check types) - Dashboard badges and project list badges Infrastructure: QaE2eTestData with pre-persisted QA issues, TranslationQaIssueBuilder, QaE2eDataController, QA_CHECKS feature flag enabled for E2E, data-cy on QaBadge component. Also includes Boy Scout improvements in TestDataService (removed no-op map, filterNotNull→mapNotNull, merged double iteration) and QaBadge (CSS dedup, simplified condition).
| @Enumerated(EnumType.STRING) | ||
| @ColumnDefault("'OPEN'") | ||
| var state: QaIssueState = QaIssueState.OPEN, | ||
| @Column(columnDefinition = "text") |
There was a problem hiding this comment.
how about to consider using jsonb ? ProjectQaConfig.settings already does have this
There was a problem hiding this comment.
Hmm, must have missed that. Thanks!
- Fix IDOR vulnerability: validate key belongs to project via KeyService.get(projectId, id) before using keyId in WebSocket handler - Add server-side rate limiting on QA preview WebSocket (10 msg/s) - Add WebSocket reconnection with exponential backoff and disconnected indicator in the frontend - Fix CoroutineScope lifecycle leak: cancel on Spring context shutdown - Chunk batch job targets at 10k to prevent OOM on large rechecks - Route LanguageStatsService through TranslationQaIssueService - Handle NotFoundException separately in WebSocket init - Add IDOR security boundary test
Both highlight components now use text-decoration-thickness: 2px for visual consistency. Removed unused QaIssueModel type alias.
Use LinkExternal instead of react-router Link for the QA settings link in the QA panel, matching the glossaries panel pattern.
When a project has ICU placeholders enabled, curly braces are part of the ICU syntax and should not be checked by bracket mismatch/balance checks. This avoids duplicate noise alongside the dedicated placeholder and ICU syntax checks.
| }); | ||
|
|
||
| it('shows badge on translation cell with QA issues', () => { | ||
| login('test_username'); |
There was a problem hiding this comment.
could be in beforeEach, see tests in other files too
| qaTestData.clean(); | ||
| }); | ||
|
|
||
| it('shows QA badge on language row in dashboard', () => { |
There was a problem hiding this comment.
these two tests could be joined into one test
| const view = new E2TranslationsView(); | ||
| view.visit(projectId); | ||
|
|
||
| cy.contains('key_multiple_issues') |
There was a problem hiding this comment.
we should probably follow the E2 compound approach for this type of repeatable code
this applies to all the test files in this PR
| package io.tolgee.ee.data.qa | ||
|
|
||
| data class QaCheckPreviewDone( | ||
| val type: String = "done", |
|
Good job, @Anty0! You’ve put in a lot of hard work. 💪 |
LanguageTool's CASING category rules (e.g., UPPERCASE_SENTENCE_START)
produce false positives on non-sentence translations like button labels
("save", "click here", "add new item"). Source-vs-translation casing
is already covered by CharacterCaseMismatchCheck.
Empirically verified across EN/DE/FR/CS that only CASING rules fire
as false positives — no PUNCTUATION or other categories affected.
Add "Read more" links to the spelling and grammar QA check tooltips in project QA settings, pointing to documentation about check limitations and supported languages.
LanguageTool flags ICU placeholders, HTML tags, and URLs as spelling or grammar errors. Filter out results whose position overlaps with these elements since dedicated checks already cover them. Extract UrlExtractor and PlaceholderExtractor as shared utilities from DifferentUrlsCheck and InconsistentPlaceholdersCheck companion objects. Rename isSpellingRule.kt and isSentenceRule.kt to PascalCase for consistency.
- Add QA_ISSUE_IGNORE and QA_ISSUE_UNIGNORE activity types - Add activity logging annotations to TranslationQaIssue entity - Add @RequestActivity to QaIssueController endpoints - Set disableActivityLogging on entities in replaceIssuesForTranslation - Add frontend activity configuration, entity display, and state formatter - Fix isVirtual logic in ignoreIssueByParams for accurate analytics - Add @transactional(readOnly = true) to read-only query methods - Add activity log tests for ignore, unignore, suppress, and batch
…ions New QA check (TRIM_CHECK) that runs only on base translations and flags leading/trailing spaces and newlines. Users can ignore (accepting the whitespace so comparison checks enforce it) or correct (removing it). Also extracts shared whitespace utilities into checks/whitespace/ package, renames QaCheckCategory.COMPARISON to NORMAL, and extracts doubled-spaces regex to companion object in SpacesMismatchCheck.
…propagation Move the enable/disable switch and title outside the disabled area so they remain interactive when QA is off. Replace pointerEvents + opacity CSS hack with native MUI disabled props on Select and IconButton components. Keep visual dimming (opacity) on the settings body for UX clarity.
…flag The getKeyLanguagePairsForRecheck query crashed with "SqmRoot not yet resolved to TableGroup" because the Criteria API .on() clause referenced a free query root. Fixed by joining Language through Key→Project→Languages (a SetJoin), matching the pattern used in ExportDataProvider. Also fixed qaChecksStale defaulting to false — it should be true so new and migrated translations are picked up when QA is first enabled.
The batch recheck option only checked the org-level QA_CHECKS feature flag but not the per-project useQaChecks toggle, so it remained visible even when QA was disabled for the project.
…plify event-driven parts Replaced multiple event classes and listeners for QA-related activities (e.g., text modifications, project creation) with a consolidated `QaActivityListener`.
Move all qaChecksStale=true marking into QaActivityListener as the single source of truth. Previously scattered across 8+ locations (SetTranslationTextUtil, Translation.clear(), BranchMergeExecutor, AutoTranslationService, TranslationService, Key/Translation merge). Key changes: - markTranslationsStale() runs unconditionally (no feature guard, works during batch chunks) — ensures correct stale state when QA is enabled later and fixes bug where siblings of base language changes during batch ops were never marked stale - startQaCheckJobs() only runs outside batch chunks when QA enabled - onKeyMaxCharLimitChanged() kept as exception (text doesn't change)
Move maxCharLimit change handling from direct service calls in KeyService and KeyComplexEditHelper into the centralized QaActivityListener, consistent with how translation text, language tag, and base language changes are already handled. Also: - Add setQaChecksStaleByKeyIds for efficient bulk stale marking by key - Extract startQaCheckBatchJob helper to deduplicate batch job logic - Remove unused checkTranslationsEditPermission extension - Add explicit parentheses to isIsPluralChanged condition
Replace the two-step pattern (fetch sibling IDs → mark stale) with a single UPDATE query in TranslationService. Extract shared getTextChangedTranslations() helper to deduplicate filter logic.
2 participants
Summary
Built-in QA checks that detect translation quality issues (missing numbers, punctuation mismatches, spacing problems, case mismatches, empty translations) directly in the Tolgee UI.
Pitch: #2858
Building plan: #2858 (comment)
Summary by CodeRabbit
New Features
UI
Tests
Chores