fix: false QA checks on numbers#3788
Conversation
📝 WalkthroughWalkthroughModifies MissingNumbersCheck to normalize extracted numbers by removing commas before comparison and reworks number extraction to use a lenient block regex followed by digit filtering and strict regex validation. Adds tests verifying grouped numbers with different formatting are not flagged as missing. ChangesMissing Number Check Normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)Not applicable. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt (1)
33-34: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffDot-grouped locales (e.g. de-DE "2.000") aren't normalized.
Only commas are stripped before comparison. Locales that use
.as a thousands separator (producing an extracted value like"2.000") won't match a plain"2000"from another locale, sinceNUMBER_REGEXtreats the dot as a valid decimal/group separator and it survives extraction unchanged. This may be intentional (dot is ambiguous between decimal point and grouping separator), but it leaves a related false-positive gap unaddressed by this fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt` around lines 33 - 34, The number normalization in MissingNumbersCheck currently strips only commas, so dot-grouped values like “2.000” can still fail to match “2000”. Update the normalization logic around baseNumbersNormalized and textNumbersNormalized to also handle dot-grouped thousands separators in a safe way, ideally in a shared helper near NUMBER_REGEX, while preserving genuine decimal values and keeping the comparison behavior consistent across locales.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt`:
- Around line 29-38: The number comparison in MissingNumbersCheck is still
order-sensitive because it gates normalization on baseNumbersNormalized ==
textNumbersNormalized, so reordered or partially matching numeric strings can
skip comma stripping entirely. Update the logic around extractNumbers and the
baseNumbersNormalized/textNumbersNormalized handling to compare numbers
independently rather than as full ordered lists, ideally by matching
comma-stripped values with a multiset-style approach. Keep the normalization
applied per number so equivalent values are recognized even when their order
differs.
- Around line 61-78: The LENIENT_BLOCK_REGEX in
MissingNumbersCheck.extractNumbers is too broad and merges unrelated numeric
tokens like ranges, times, and dates into one block. Tighten the separator
handling so only valid numeric grouping/decimal characters are accepted, or
explicitly document and test the broader behavior if it is intentional. Update
the logic around LENIENT_BLOCK_REGEX and NUMBER_REGEX, and add coverage for
cases like 3-4, 10:30, and 01/02 to prevent regressions.
---
Nitpick comments:
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt`:
- Around line 33-34: The number normalization in MissingNumbersCheck currently
strips only commas, so dot-grouped values like “2.000” can still fail to match
“2000”. Update the normalization logic around baseNumbersNormalized and
textNumbersNormalized to also handle dot-grouped thousands separators in a safe
way, ideally in a shared helper near NUMBER_REGEX, while preserving genuine
decimal values and keeping the comparison behavior consistent across locales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1947a79a-b7e9-4561-ab2a-8268bb896a65
📒 Files selected for processing (2)
ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.ktee/backend/tests/src/test/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheckTest.kt
| var baseNumbers = extractNumbers(base) | ||
| var textNumbers = extractNumbers(text) | ||
|
|
||
| //compare both numbers if they have comma then make them same and then compare | ||
| val baseNumbersNormalized = baseNumbers.map { it.replace(",", "") } | ||
| val textNumbersNormalized = textNumbers.map { it.replace(",", "") } | ||
|
|
||
| if(baseNumbersNormalized == textNumbersNormalized) { | ||
| baseNumbers = textNumbers | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ordered-list comparison can miss reordered/formatting-equivalent numbers.
baseNumbersNormalized == textNumbersNormalized is a positional list comparison. If the translation reorders numeric mentions (common across locales) or if any other number differs elsewhere in the string, the whole-array equality check fails and no normalization is applied at all — even for numbers that would otherwise be equivalent after comma-stripping. This re-introduces the false-positive class the PR is meant to fix for anything beyond exact-order, exact-count matches.
Consider normalizing each number independently (e.g., build multisets keyed on the comma-stripped value) instead of gating on whole-list equality:
♻️ Suggested normalization approach
- var baseNumbers = extractNumbers(base)
- var textNumbers = extractNumbers(text)
-
- //compare both numbers if they have comma then make them same and then compare
- val baseNumbersNormalized = baseNumbers.map { it.replace(",", "") }
- val textNumbersNormalized = textNumbers.map { it.replace(",", "") }
-
- if(baseNumbersNormalized == textNumbersNormalized) {
- baseNumbers = textNumbers
- }
-
- val baseMultiset = baseNumbers.groupingBy { it }.eachCount()
- val textMultiset = textNumbers.groupingBy { it }.eachCount()
+ val baseNumbers = extractNumbers(base).map { it.replace(",", "") }
+ val textNumbers = extractNumbers(text).map { it.replace(",", "") }
+
+ val baseMultiset = baseNumbers.groupingBy { it }.eachCount()
+ val textMultiset = textNumbers.groupingBy { it }.eachCount()📝 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.
| var baseNumbers = extractNumbers(base) | |
| var textNumbers = extractNumbers(text) | |
| //compare both numbers if they have comma then make them same and then compare | |
| val baseNumbersNormalized = baseNumbers.map { it.replace(",", "") } | |
| val textNumbersNormalized = textNumbers.map { it.replace(",", "") } | |
| if(baseNumbersNormalized == textNumbersNormalized) { | |
| baseNumbers = textNumbers | |
| } | |
| val baseNumbers = extractNumbers(base).map { it.replace(",", "") } | |
| val textNumbers = extractNumbers(text).map { it.replace(",", "") } | |
| val baseMultiset = baseNumbers.groupingBy { it }.eachCount() | |
| val textMultiset = textNumbers.groupingBy { it }.eachCount() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt`
around lines 29 - 38, The number comparison in MissingNumbersCheck is still
order-sensitive because it gates normalization on baseNumbersNormalized ==
textNumbersNormalized, so reordered or partially matching numeric strings can
skip comma stripping entirely. Update the logic around extractNumbers and the
baseNumbersNormalized/textNumbersNormalized handling to compare numbers
independently rather than as full ordered lists, ideally by matching
comma-stripped values with a multiset-style approach. Keep the normalization
applied per number so equivalent values are recognized even when their order
differs.
| private val LENIENT_BLOCK_REGEX = Regex("""\d+(?:[^0-9]\d+)*""") | ||
| private val NUMBER_REGEX = Regex("""\d+([.,]\d+)*""") | ||
|
|
||
| fun extractNumbers(text: String): List<String> { | ||
| return NUMBER_REGEX.findAll(text).map { it.value }.toList() | ||
| return LENIENT_BLOCK_REGEX.findAll(text).mapNotNull { matchResult -> | ||
| val rawBlock = matchResult.value | ||
|
|
||
| // Step 2: Cycle through and keep ONLY digits, periods, and commas | ||
| val filteredString = rawBlock.filter { it.isDigit() || it == '.' || it == ',' } | ||
|
|
||
| // Step 3: Pass the filtered string through your target regex | ||
| // If it matches completely, we return the value; otherwise, we discard it (mapNotNull) | ||
| if (NUMBER_REGEX.matches(filteredString)) { | ||
| filteredString | ||
| } else { | ||
| null | ||
| } | ||
| }.toList() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Target file ==\n'
sed -n '1,220p' ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt
printf '\n== Search usages/tests ==\n'
rg -n "MissingNumbersCheck|extractNumbers\\(" ee/backend/app/src -g '*.{kt,kts}'
printf '\n== Candidate test files ==\n'
fd -a "MissingNumbersCheck" ee/backend/app/src/test ee/backend/app/src/main || trueRepository: tolgee/tolgee-platform
Length of output: 3704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Repo files matching MissingNumbersCheck / QA number tests ==\n'
git ls-files | rg 'MissingNumbersCheck|qa|QA|Number|Numbers|MissingNumbers'
printf '\n== Search for extractNumbers test references ==\n'
rg -n "extractNumbers\\(|MISSING_NUMBERS|QA_NUMBERS_MISSING|MissingNumbersCheck" . -g '*.{kt,kts,java,ts,tsx,js,jsx}'
printf '\n== Show nearby QA check files ==\n'
git ls-files ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks | sed -n '1,120p'Repository: tolgee/tolgee-platform
Length of output: 19783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file='ee/backend/tests/src/test/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheckTest.kt'
wc -l "$test_file"
sed -n '1,220p' "$test_file"Repository: tolgee/tolgee-platform
Length of output: 2999
🏁 Script executed:
python3 - <<'PY'
import re
LENIENT_BLOCK_REGEX = re.compile(r"\d+(?:[^0-9]\d+)*")
NUMBER_REGEX = re.compile(r"\d+([.,]\d+)*")
def extract_numbers(text):
out = []
for m in LENIENT_BLOCK_REGEX.finditer(text):
raw = m.group(0)
filtered = ''.join(ch for ch in raw if ch.isdigit() or ch in '.,')
if NUMBER_REGEX.fullmatch(filtered):
out.append(filtered)
return out
samples = [
"2 000",
"2_0_00",
"1,000",
"9.99",
"3-4",
"16:9",
"10:30",
"01/02",
"2024-07-03",
"abc 3-4 def 5",
]
for s in samples:
print(f"{s!r} -> {extract_numbers(s)}")
PYRepository: tolgee/tolgee-platform
Length of output: 375
Lenient block regex merges unrelated numeric tokens. \d+(?:[^0-9]\d+)* also collapses 3-4, 10:30, 01/02, etc. into a single number, not just grouping separators. Restrict the allowed separators to actual numeric grouping/decimal characters, or add coverage for these cases if this broader behavior is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/service/qa/checks/MissingNumbersCheck.kt`
around lines 61 - 78, The LENIENT_BLOCK_REGEX in
MissingNumbersCheck.extractNumbers is too broad and merges unrelated numeric
tokens like ranges, times, and dates into one block. Tighten the separator
handling so only valid numeric grouping/decimal characters are accepted, or
explicitly document and test the broader behavior if it is intentional. Update
the logic around LENIENT_BLOCK_REGEX and NUMBER_REGEX, and add coverage for
cases like 3-4, 10:30, and 01/02 to prevent regressions.
1 participant
The rationale behind the code changes is not to find all the special characters and handle it explicitly within the code but rather strip all the unnecessary characters encountered between the numbers (except
.,) and create a normalized string which are then compared to find the errors.The algorithm works as follows:
Resolves #3753
Summary by CodeRabbit