Skip to content

[PM-42523] fix: Restore AppIntents localization strings and detect their usage - #3005

Open
fedemkr wants to merge 2 commits into
mainfrom
PM-42523/fix-app-intents-localizations
Open

[PM-42523] fix: Restore AppIntents localization strings and detect their usage#3005
fedemkr wants to merge 2 commits into
mainfrom
PM-42523/fix-app-intents-localizations

Conversation

@fedemkr

@fedemkr fedemkr commented Aug 28, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-42523

📔 Objective

PM-26292 ("chore: Remove unused strings", #2003) removed 10 Localizable.strings entries that are actually referenced by Bitwarden/Application/AppIntents/*.swift and ShortcutsProvider.swift — but only as raw string literals (title, description, shortTitle, dialog, and a CustomLocalizedStringResourceConvertible.localizedStringResource switch), not via the generated Localizations.X enum. Without a matching entry, Siri/Shortcuts falls back to showing the raw key text instead of the intended English string.

This PR:

  1. Restores the 10 missing entries in BitwardenResources/Localizations/en.lproj/Localizable.strings, at their original positions, with values matching what was deleted.
  2. Fixes the root cause in Scripts/fix-localizable-strings/delete_unused_strings.py (used by the weekly cron-fix-localizable-strings GitHub Action) so its delete-unused detector recognizes these AppIntents literal-key usage patterns as "used" — otherwise the bot would silently delete them again on its next run. Adds test coverage for each pattern, including the actual AppIntentError shape in AppIntentMediator.swift.

Verified via bash Scripts/fix-localizable-strings.sh --dry-run, which now reports no unused strings in Localizable.strings, and the full Python test suite (Scripts/test-fix-localizable-strings.sh, 88/88 passing).

@fedemkr fedemkr added the ai-review Request a Claude code review label Aug 28, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:bug Change Type - Bug labels Aug 28, 2026
@fedemkr
fedemkr marked this pull request as ready for review August 28, 2026 21:24
@fedemkr
fedemkr requested review from a team and matt-livefront as code owners August 28, 2026 21:24
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the restoration of 11 Localizable.strings entries and the delete_unused_strings.py detector fix that prevents the weekly cron job from deleting them again. Every restored key and value was diffed against the original removal in ba7673872 — keys, values, and file positions all match exactly, and the two genuinely-unused keys removed in that commit (LockCurrentAccount, AnErrorOccurredWhileTryingToLockAllAccounts) were correctly left out. The new regexes and the LocalizedStringResource { ... } brace-walking extractor cover every literal-key site in Bitwarden/Application/AppIntents/*.swift and AppIntentMediator.swift, and Bitwarden/ is already in SWIFT_SOURCE_DIRS so the detector reaches them; the change can only add keys to the used set, so it fails safe toward retention. Also confirmed project-pm.yml:172 includes BitwardenResources/Localizations in the main app target, so these keys resolve from Bundle.main where LocalizedStringResource looks for them.

Code Review Details
  • 🎨 : Title regex arm requires static var; a static let title migration would silently regress the fix
    • Scripts/fix-localizable-strings/delete_unused_strings.py:46-53

Comment on lines +46 to +53
_APPINTENT_LITERAL_KEY_RE = re.compile(
r'(?:'
r'static\s+var\s+title\s*:\s*LocalizedStringResource\s*=\s*'
r'|IntentDescription\(\s*'
r'|shortTitle\s*:\s*'
r'|dialog\s*:\s*'
r')"([^"\\]*)"'
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: The title arm requires static var, so a static let title would silently regress this fix.

Details and fix

All four current intents use static var title: LocalizedStringResource = "Key", so the regex matches today. But static var on an AppIntent is exactly the declaration Swift 6 strict-concurrency checking pushes toward static let. If any intent is migrated (or a new one is written that way), the key stops being detected and the weekly cron-fix-localizable-strings job deletes the entry again — the same silent failure this PR is fixing.

Anchoring on title: instead of static var title covers static var, static let, and non-static forms with no added false-match surface:

Suggested change
_APPINTENT_LITERAL_KEY_RE = re.compile(
r'(?:'
r'static\s+var\s+title\s*:\s*LocalizedStringResource\s*=\s*'
r'|IntentDescription\(\s*'
r'|shortTitle\s*:\s*'
r'|dialog\s*:\s*'
r')"([^"\\]*)"'
)
_APPINTENT_LITERAL_KEY_RE = re.compile(
r'(?:'
r'title\s*:\s*LocalizedStringResource\s*=\s*'
r'|IntentDescription\(\s*'
r'|shortTitle\s*:\s*'
r'|dialog\s*:\s*'
r')"([^"\\]*)"'
)

Note the LocalizedStringResource { ... } block extractor does not cover this case either, since it only matches a brace-opened body, not an = "literal" assignment.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.51%. Comparing base (2c24d3f) to head (9f3b8b7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3005      +/-   ##
==========================================
- Coverage   79.54%   79.51%   -0.03%     
==========================================
  Files        1169     1169              
  Lines       75095    75095              
==========================================
- Hits        59731    59711      -20     
- Misses      15364    15384      +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant