Fix false positive translate prompts on short English text#3673
Open
alltheseas wants to merge 1 commit intodamus-io:masterfrom
Open
Fix false positive translate prompts on short English text#3673alltheseas wants to merge 1 commit intodamus-io:masterfrom
alltheseas wants to merge 1 commit intodamus-io:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors NdbNote.note_language to use cleaned content and NLLanguageRecognizer with short-text and confidence heuristics, and adds LanguageDetectionTests plus Xcode project wiring to run them. Changes
Sequence Diagram(s)sequenceDiagram
participant Keypair
participant NdbNote
participant NLLanguageRecognizer
participant LocaleLookup
rect rgba(200,200,255,0.5)
Keypair->>NdbNote: get_content(keypair)
NdbNote->>NdbNote: trimmedText = clean(content)
alt trimmedText empty or short Latin-only (<11)
NdbNote->>NdbNote: return nil
else
NdbNote->>NLLanguageRecognizer: analyze(trimmedText, maxHypotheses=3)
NLLanguageRecognizer-->>NdbNote: hypotheses + probabilities
alt trimmedText.length < 60 and locale present (prob≥0.1)
NdbNote->>LocaleLookup: currentLocaleLanguage
NdbNote->>NdbNote: return locale language
else
NdbNote->>NdbNote: pick best hypothesis (prob≥0.5)
NdbNote->>NdbNote: normalize via localeToLanguage(...)
NdbNote->>NdbNote: return detected language or nil
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nostrdb/NdbNote.swift (1)
584-625: Add a doc comment for the newnote_language(_:)contract.This method now has important non-obvious rules:
< 11returnsnil,< 60may return the current locale language from a secondary hypothesis, and callers must keep it off the main thread. That behavior should be documented on the API.As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nostrdb/NdbNote.swift` around lines 584 - 625, Add a doc comment above note_language(_:) describing its behavior and constraints: state that callers must not call it on the main thread (assert), explain that it returns nil for empty text or text shorter than 11 characters, that for texts shorter than 60 characters it may return the user's current locale language if that locale appears as a hypothesis with confidence >= 0.1, and that for longer texts it returns the top hypothesis only when its confidence >= 0.5; include mention of the return type (optional String) and purpose (language detection for UI/translation decisions).damusTests/LanguageDetectionTests.swift (1)
241-254: Use the production API for the “after” result instead of re-implementing it here.
newResultcopies the currentnote_languagelogic, so this test can keep passing if the real implementation later drifts on trimming, locale normalization, or content extraction. Keeping the old baseline inline is fine, but the post-fix value should come from the shipped API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damusTests/LanguageDetectionTests.swift` around lines 241 - 254, The test is re-implementing production logic in the newResult closure; instead call the shipped API (note_language or the equivalent production language-detection function) instead of duplicating logic. Replace the inline computation of newResult (which uses text, hyps, currentLang) with a call to the production note_language API, passing the same input text/metadata so the test compares against the real “after” result and stays aligned with trimming, locale normalization, and extraction logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@damusTests/LanguageDetectionTests.swift`:
- Around line 136-172: The tests assume an English runner but note_language
consults Locale.current for short texts; pin the runner language in the test by
setting and restoring the process language before running assertions (e.g.
capture previous AppleLanguages = UserDefaults.standard.array(forKey:
"AppleLanguages"), set UserDefaults.standard.set(["en"], forKey:
"AppleLanguages") and synchronize, run the loop that calls
test_shortEnglishPhrasesNotDetectedAsForeign -> NostrEvent and note_language,
then restore the saved AppleLanguages), and apply the same change to the other
test block referenced (lines ~214-283); reference functions/places:
testShortEnglishPhrasesNotDetectedAsForeign, note_language, and NdbNote.swift
where Locale.current is used.
In `@nostrdb/NdbNote.swift`:
- Around line 587-605: The length checks and language detection currently run on
raw note content from get_content(keypair), which can include URLs, mentions,
and hashtags; update the flow to first produce a text-only version (strip URLs,
`@mentions`, and `#hashtags` similar to the logic in
damus/Core/Nostr/NostrEvent.swift), then apply the short-text guards (the >=11
and other length gates) and call NLLanguageRecognizer.processString on that
cleaned text-only string; ensure functions referenced (get_content(keypair),
NLLanguageRecognizer.processString, and the short-text guard checks) use the
stripped text so links/handles don't cause false positives for the translate
prompt.
---
Nitpick comments:
In `@damusTests/LanguageDetectionTests.swift`:
- Around line 241-254: The test is re-implementing production logic in the
newResult closure; instead call the shipped API (note_language or the equivalent
production language-detection function) instead of duplicating logic. Replace
the inline computation of newResult (which uses text, hyps, currentLang) with a
call to the production note_language API, passing the same input text/metadata
so the test compares against the real “after” result and stays aligned with
trimming, locale normalization, and extraction logic.
In `@nostrdb/NdbNote.swift`:
- Around line 584-625: Add a doc comment above note_language(_:) describing its
behavior and constraints: state that callers must not call it on the main thread
(assert), explain that it returns nil for empty text or text shorter than 11
characters, that for texts shorter than 60 characters it may return the user's
current locale language if that locale appears as a hypothesis with confidence
>= 0.1, and that for longer texts it returns the top hypothesis only when its
confidence >= 0.5; include mention of the return type (optional String) and
purpose (language detection for UI/translation decisions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d9afac-78b7-41ad-9f3b-424c3a30ee65
📒 Files selected for processing (3)
damus.xcodeproj/project.pbxprojdamusTests/LanguageDetectionTests.swiftnostrdb/NdbNote.swift
Comment on lines
+587
to
+605
| let text = self.get_content(keypair) | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| guard !text.isEmpty else { | ||
| return nil | ||
| } | ||
| let blocks = blocks_txn.unsafeUnownedValue | ||
|
|
||
| let originalOnlyText = blocks.blocks(note: self).compactMap { | ||
| if case .text(let txt) = $0 { | ||
| // Replacing right single quotation marks (’) with "typewriter or ASCII apostrophes" (') | ||
| // as a workaround to get Apple's language recognizer to predict language the correctly. | ||
| // It is important to add this workaround to get the language right because it wastes users' money to send translation requests. | ||
| // Until Apple fixes their language model, this workaround will be kept in place. | ||
| // See https://en.wikipedia.org/wiki/Apostrophe#Unicode for an explanation of the differences between the two characters. | ||
| // | ||
| // For example, | ||
| // "nevent1qqs0wsknetaju06xk39cv8sttd064amkykqalvfue7ydtg3p0lyfksqzyrhxagf6h8l9cjngatumrg60uq22v66qz979pm32v985ek54ndh8gj42wtp" | ||
| // has the note content "It’s a meme". | ||
| // Without the character replacement, it is 61% confident that the text is in Turkish (tr) and 8% confident that the text is in English (en), | ||
| // which is a wildly incorrect hypothesis. | ||
| // With the character replacement, it is 65% confident that the text is in English (en) and 24% confident that the text is in Turkish (tr), which is more accurate. | ||
| // | ||
| // Similarly, | ||
| // "nevent1qqspjqlln6wvxrqg6kzl2p7gk0rgr5stc7zz5sstl34cxlw55gvtylgpp4mhxue69uhkummn9ekx7mqpr4mhxue69uhkummnw3ez6ur4vgh8wetvd3hhyer9wghxuet5qy28wumn8ghj7un9d3shjtnwdaehgu3wvfnsygpx6655ve67vqlcme9ld7ww73pqx7msclhwzu8lqmkhvuluxnyc7yhf3xut" | ||
| // has the note content "You’re funner". | ||
| // Without the character replacement, it is 52% confident that the text is in Norwegian Bokmål (nb) and 41% confident that the text is in English (en). | ||
| // With the character replacement, it is 93% confident that the text is in English (en) and 4% confident that the text is in Norwegian Bokmål (nb). | ||
| return txt.replacingOccurrences(of: "’", with: "'") | ||
| } | ||
| else { | ||
| return nil | ||
| } | ||
| } | ||
| .joined(separator: " ") | ||
| */ | ||
|
|
||
| let originalOnlyText = self.get_content(keypair) | ||
|
|
||
| // If there is no text, there's nothing to use to detect language. | ||
| guard !originalOnlyText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { | ||
| // Very short text (single words, abbreviations) is too ambiguous for | ||
| // reliable language detection. Return nil to suppress the translate | ||
| // button rather than risk a false positive. | ||
| guard text.count >= 11 else { | ||
| return nil | ||
| } | ||
|
|
||
| let languageRecognizer = NLLanguageRecognizer() | ||
| languageRecognizer.processString(originalOnlyText) | ||
| languageRecognizer.processString(text) | ||
|
|
||
| let hypotheses = languageRecognizer.languageHypotheses(withMaximum: 3) | ||
|
|
Contributor
There was a problem hiding this comment.
Keep the new length gates on text-only content, not raw note content.
get_content(keypair) still includes URLs, hashtags, and mentions, so non-linguistic tokens can push a note past the 11/60-character guards and change the recognizer result. The existing damus/Core/Nostr/NostrEvent.swift path at Lines 324-342 strips those pieces before detection for exactly that reason. Apply the short-text checks and processString to text-only blocks here as well, or short notes with links can still surface a false translate prompt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nostrdb/NdbNote.swift` around lines 587 - 605, The length checks and language
detection currently run on raw note content from get_content(keypair), which can
include URLs, mentions, and hashtags; update the flow to first produce a
text-only version (strip URLs, `@mentions`, and `#hashtags` similar to the logic in
damus/Core/Nostr/NostrEvent.swift), then apply the short-text guards (the >=11
and other length gates) and call NLLanguageRecognizer.processString on that
cleaned text-only string; ensure functions referenced (get_content(keypair),
NLLanguageRecognizer.processString, and the short-text guard checks) use the
stripped text so links/handles don't cause false positives for the translate
prompt.
alltheseas
force-pushed
the
fix/language-detection-false-positives
branch
from
d9a52fc to
81bc854
Compare
NLLanguageRecognizer misidentifies short English text (e.g. "gut" → German,
"vast" → Dutch, "cap" → Catalan), causing a spurious "translate note" button.
Two-layer fix in note_language():
1. Return nil for short Latin-script text (< 11 chars) where detection is
unreliable. Non-Latin scripts (CJK, Cyrillic, Arabic, etc.) bypass this
guard because the writing system itself is a strong signal.
2. For text < 60 chars, check if user's language appears in top 3 hypotheses
with >= 10% confidence before falling back to the old 50% threshold.
Short Latin-script foreign words ("hola", "ciao") are intentionally suppressed:
at 4-5 characters these are genuinely ambiguous (commonly borrowed into English)
and NLLanguageRecognizer confidence is too low to be actionable.
Tested against 107 sample notes (28 confirmed false positives + 42 additional
English + 7 short foreign + 30 longer foreign): false positives drop from 28
to 0, with 0 false negatives introduced. Short non-Latin foreign text (e.g.
"Привет", "こんにちは", "你好") is still correctly detected.
Changelog-Fixed: Fix translate button appearing on short English notes
Closes: damus-io#3671
Signed-off-by: alltheseas <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Signed-off-by: alltheseas <[email protected]>
alltheseas
force-pushed
the
fix/language-detection-false-positives
branch
from
81bc854 to
79e9067
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nostrdb/NdbNote.swift (1)
597-621:⚠️ Potential issue | 🟠 MajorApply the heuristics to text-only blocks.
get_content(keypair)still includes URLs, hashtags, and mentions, so non-linguistic tokens can push a short note past the< 11/< 60guards and skewNLLanguageRecognizer. The existingdamus/Core/Nostr/NostrEvent.swiftpath strips to text blocks before detection; please do the same here before trimming, counting, and callingprocessString.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nostrdb/NdbNote.swift` around lines 597 - 621, The language-detection heuristics are being run on raw output from get_content(keypair) which still contains URLs, mentions, and hashtags; before trimming, length-checks (<11 / <60) and calling NLLanguageRecognizer.processString you should first extract the plain text blocks (strip URLs, `@mentions`, `#hashtags`—same approach used in damus/Core/Nostr/NostrEvent.swift) and assign that cleaned text to the local text variable; then run the existing non-Latin detection, length guards, and languageRecognizer.processString on the cleaned text so non-linguistic tokens don’t skew detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@damusTests/LanguageDetectionTests.swift`:
- Around line 12-23: Add Swift docstrings (///) for the new test scaffolding:
place a brief docstring above the LanguageDetectionTests class describing its
purpose, above the static property sampleTexts explaining the tuple contents and
detection expectations (english vs foreign and expectedLangs behavior), and
above the padRow helper describing its role in preparing test data; use the
existing prose comments as the docstring text so the file meets the Swift
documentation rule and stays compliant with coding guidelines.
- Around line 167-177: The test is exercising NostrEvent.note_language but the
real logic moved to NdbNote.note_language; change the test to build an NdbNote
(or call the shared detector used by NdbNote) instead of constructing NostrEvent
so the suite exercises nostrdb behavior: replace the NostrEvent creation and the
call to event.note_language(test_keypair) with code that constructs the NdbNote
using the same content/keypair/createdAt and then calls NdbNote.note_language
(or the shared detector function), and make the identical swap at the other two
call sites that the reviewer noted (the blocks around the other two
occurrences).
---
Duplicate comments:
In `@nostrdb/NdbNote.swift`:
- Around line 597-621: The language-detection heuristics are being run on raw
output from get_content(keypair) which still contains URLs, mentions, and
hashtags; before trimming, length-checks (<11 / <60) and calling
NLLanguageRecognizer.processString you should first extract the plain text
blocks (strip URLs, `@mentions`, `#hashtags`—same approach used in
damus/Core/Nostr/NostrEvent.swift) and assign that cleaned text to the local
text variable; then run the existing non-Latin detection, length guards, and
languageRecognizer.processString on the cleaned text so non-linguistic tokens
don’t skew detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecc2a51b-290a-4d2d-985f-cd3a3a54a815
📒 Files selected for processing (3)
damus.xcodeproj/project.pbxprojdamusTests/LanguageDetectionTests.swiftnostrdb/NdbNote.swift
Comment on lines
+12
to
+23
| final class LanguageDetectionTests: XCTestCase { | ||
|
|
||
| // 107 sample texts: (text, isEnglish, expectedLangs) | ||
| // | ||
| // English texts should be detected as "en" or nil, never as a foreign language. | ||
| // Foreign texts should be detected as a non-English language. When expectedLangs | ||
| // is non-empty, the detected language must be in that set (accounts for closely | ||
| // related languages that NLLanguageRecognizer may confuse, e.g. ru/bg/uk). | ||
| // | ||
| // The English set includes 28 phrases confirmed to trigger false positives | ||
| // under the old single-hypothesis 50% threshold logic on iOS 26.2. | ||
| static let sampleTexts: [(text: String, isEnglish: Bool, expectedLangs: Set<String>)] = [ |
Contributor
There was a problem hiding this comment.
Add docstrings for the new test scaffolding.
The new test case, shared dataset, and padRow helper are added without /// docs. Please promote the existing prose comments to docstrings so this file stays compliant with the Swift documentation rule.
As per coding guidelines, "Ensure docstring coverage for any code added or modified"
Also applies to: 316-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damusTests/LanguageDetectionTests.swift` around lines 12 - 23, Add Swift
docstrings (///) for the new test scaffolding: place a brief docstring above the
LanguageDetectionTests class describing its purpose, above the static property
sampleTexts explaining the tuple contents and detection expectations (english vs
foreign and expectedLangs behavior), and above the padRow helper describing its
role in preparing test data; use the existing prose comments as the docstring
text so the file meets the Swift documentation rule and stays compliant with
coding guidelines.
Comment on lines
+167
to
+177
| guard let event = NostrEvent( | ||
| content: text, | ||
| keypair: test_keypair, | ||
| createdAt: UInt32(Date().timeIntervalSince1970) | ||
| ) else { | ||
| XCTFail("Could not create event for '\(text)'") | ||
| continue | ||
| } | ||
|
|
||
| let lang = event.note_language(test_keypair) | ||
|
|
Contributor
There was a problem hiding this comment.
Exercise NdbNote.note_language in this suite.
These tests still build a NostrEvent and call its separate note_language implementation, but the behavior changed in this PR lives in nostrdb/NdbNote.swift. Since the two paths already preprocess content differently, this suite can go green while the shipped nostrdb path still regresses. Please build an NdbNote here, or extract a shared detector and test that instead.
🔧 Minimal fix
- guard let event = NostrEvent(
+ guard let note = NdbNote(
content: text,
keypair: test_keypair,
createdAt: UInt32(Date().timeIntervalSince1970)
) else {
- XCTFail("Could not create event for '\(text)'")
+ XCTFail("Could not create note for '\(text)'")
continue
}
- let lang = event.note_language(test_keypair)
+ let lang = note.note_language(test_keypair)Apply the same swap in the other two call sites above.
Based on learnings, "Add or update unit tests in damusTests/ alongside feature changes, especially when touching parsing, storage, or replay logic"
Also applies to: 208-217, 273-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damusTests/LanguageDetectionTests.swift` around lines 167 - 177, The test is
exercising NostrEvent.note_language but the real logic moved to
NdbNote.note_language; change the test to build an NdbNote (or call the shared
detector used by NdbNote) instead of constructing NostrEvent so the suite
exercises nostrdb behavior: replace the NostrEvent creation and the call to
event.note_language(test_keypair) with code that constructs the NdbNote using
the same content/keypair/createdAt and then calls NdbNote.note_language (or the
shared detector function), and make the identical swap at the other two call
sites that the reviewer noted (the blocks around the other two occurrences).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NLLanguageRecognizer misidentifies short English text (e.g. "gut" → German, "vast" → Dutch, "cap" → Catalan), causing a spurious "translate note" button to appear for same-language readers.
Two-layer fix in
note_language():nilfor text < 11 chars — too ambiguous for reliable language detection in any scriptWhen
note_language()returnsnil,TranslateViewhides the translate button (if let note_langguard fails), which is the correct behavior for ambiguous text.Commits (review commit-by-commit)
d9a52fc0— Fix false positive translate prompts on short English text (27 impl, 298 test)Total: 27 impl, 298 test
Standard PR Checklist
note_language()which already runs on a background thread; the added guards reduce computation for short text (early return instead of running NLLanguageRecognizer)Closes:tags in the commit messages.Test report
Device: iPhone 17 Pro Simulator
iOS: 26.2
Damus:
d9a52fc0onfix/language-detection-false-positivesSetup: Regression test suite with 100 sample texts: 28 confirmed false positives (English phrases that NLLanguageRecognizer misidentifies as foreign under the old logic) + 42 additional English phrases + 30 foreign language texts across 10 languages.
Steps:
NdbNote.swiftto master (pre-fix) and rantestShortEnglishPhrasesNotDetectedAsForeign→ FAILS with "28 English phrases falsely detected as foreign"testForeignTextsDetectedCorrectly→ PASSES (0 false negatives — all 30 foreign texts still correctly detected)testLanguageDetectionComparison→ PASSES with summary: false positives 28 → 0, false negatives 0 → 0Results:
Other notes
The 28 confirmed false positives and their old misdetections:
All 28 now correctly return
nil(translate button hidden) oren(user's language), with zero false negatives introduced.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests