Localize remaining OTPKit strings and OTP wire tokens - #149
Conversation
Extract every remaining hardcoded user-facing string in OTPKit into the
13-locale Localizable.strings tables. The demo app is deliberately out of
scope. 180 keys per locale, all referenced, none orphaned.
Notable pieces:
- SwiftUI's LocalizedStringKey initializers resolve against the *main* app
bundle, so `Text("Close")` in a Swift package never reached the package's
tables. All call sites now route through OTPLoc, which passes .module.
- Raw OTP wire tokens no longer leak into the UI. LegMode and
RelativeDirection map them to localized names, tolerating casing, spaces
and the BIKE/TRAIN aliases. An unrecognized token degrades to humanized
English and is logged, matching the convention set in #146 for
unrecognized error codes.
- TransportMode.displayName referenced four keys that existed in no table,
so users saw the literal string "transport_mode_transit". Fixed and
translated.
- The three fixed-format DateFormatters build OTP query parameters, not
display text. They are now pinned to en_US_POSIX + Gregorian + an explicit
local time zone; without the calendar pin a device on a Buddhist or
Japanese Imperial calendar emits a year the server cannot parse, and
without the zone pin iOS emits UTC and plans trips for the wrong time of
day. Both are covered by tests.
- TripOptionsSummaryView's date formatter is genuine display text and now
uses setLocalizedDateFormatFromTemplate, picking up locale field order
and 12/24-hour preference.
- Formatters.formatDateToTime built a DateFormatter per call on a path that
runs twice per itinerary row; it now reuses one, matching its siblings.
Tests cover token parsing, the wire formats, and string-table integrity
(key parity, format-specifier parity, and that every key resolves — an
unresolved key otherwise ships silently as its own name).
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughOTPKit centralizes OTP date formatting, adds typed and localized OTP token display, replaces hardcoded UI and service text with localized resources, expands translations across supported locales, and adds tests for formatting, parsing, localization integrity, and resource parity. ChangesLocalization and OTP presentation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
iOS resolves an app's language from the main bundle, so the demo — which declared no localizations — pinned itself to English and OTPKit's strings followed. The translations were bundled correctly the whole time; the host just never asked for them. Declares the 13 locales OTPKit ships via a partial Info.plist. The remaining keys are still generated by the build and merged into it. Two mechanics worth knowing if this ever needs changing: - INFOPLIST_KEY_CFBundleLocalizations does nothing. Xcode only passes through a known allow-list of INFOPLIST_KEY_* settings, and this isn't on it — the build succeeds and the key is silently absent. - The plist lives at Demo/OTPKitDemo-Info.plist, outside the target's synchronized folder group. Inside Demo/OTPKitDemo/ it gets picked up as a resource as well as an Info.plist, which fails the build with "Multiple commands produce ... Info.plist". This does not translate the demo's own UI, which stays in English.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
OTPKit/Tests/LocalizationTests.swift (1)
242-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFormat-specifier check validates count only, not type/order.
count(_:)counts specifier occurrences but doesn't compare which specifier types appear at which position. A translation that swaps%@for%d(or reorders two differing specifiers) at the same key would still pass this test, yet crash at runtime whenString(format:arguments:)receives aStringwhere anIntconversion is expected (or vice versa). Since this suite's whole purpose is guarding against exactly this class of localization bug, capturing the matched specifiers themselves (not just their count) would close the gap.♻️ Compare specifier sequences instead of counts
- func count(_ string: String) -> Int { - specifier.numberOfMatches(in: string, range: NSRange(string.startIndex..., in: string)) - } + func specifiers(_ string: String) -> [String] { + specifier.matches(in: string, range: NSRange(string.startIndex..., in: string)).map { + String(string[Range($0.range, in: string)!]) + } + } for locale in Self.locales.dropFirst() { let table = try Self.keys(in: locale) for (key, baseValue) in base { guard let translated = table[key] else { continue } - `#expect`(count(baseValue) == count(translated), + `#expect`(specifiers(baseValue) == specifiers(translated), "\(locale) \(key): \(baseValue) vs \(translated)") } }🤖 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 `@OTPKit/Tests/LocalizationTests.swift` around lines 242 - 259, Update the formatSpecifierParity test and its local count helper to extract each matched format specifier in order, then compare the resulting sequences for base and translated values. Preserve the existing locale/key iteration and expectation message, but reject translations that change specifier types or ordering rather than comparing only occurrence counts.
🤖 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 `@OTPKit/Sources/OTPKit/Core/Extensions/DateFormatterExtension.swift`:
- Line 27: The cached formatters in DateFormatterExtension.swift and
Formatters.swift must follow updated device settings instead of capturing
.current or inherited defaults. Update the formatter configuration at
DateFormatterExtension.swift:27 and Formatters.swift:89-97 to use autoupdating
locale, calendar, time zone, and clock-style settings as appropriate, preserving
each formatter’s existing formatting behavior.
In `@OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift`:
- Line 13: Make both model enums conform to Codable: update LegMode in
OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift at lines 13-13 and
RelativeDirection in
OTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swift at lines 13-13,
preserving their existing conformances.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegVehicleView.swift`:
- Around line 37-40: Update the boarding identifier in DirectionLegVehicleView
to use leg.from.stopCode instead of leg.to.stopCode. Keep the
destination/alighting section below using leg.to.stopCode.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/ItineraryDetailsView.swift`:
- Around line 68-69: Update the navigationTitle in ItineraryDetailsView to use
the existing unknownLocation fallback when destination is nil, while preserving
destination?.title for non-nil destinations and removing the separate OTPLoc
fallback.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Library/Components/CurrentLocationButton.swift`:
- Line 65: Update the CurrentLocationButton error handling so the localized
messages assigned to errorMessage in both denied and unavailable paths are
actually rendered by the view, binding errorMessage to the intended alert or
text presentation. If no presentation is intended, remove these assignments and
their unused localization keys instead.
---
Nitpick comments:
In `@OTPKit/Tests/LocalizationTests.swift`:
- Around line 242-259: Update the formatSpecifierParity test and its local count
helper to extract each matched format specifier in order, then compare the
resulting sequences for base and translated values. Preserve the existing
locale/key iteration and expectation message, but reject translations that
change specifier types or ordering rather than comparing only occurrence counts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f03c48c-3ae9-4b88-9a12-3c17c7343837
📒 Files selected for processing (52)
OTPKit/Sources/OTPKit/Core/Extensions/DateFormatterExtension.swiftOTPKit/Sources/OTPKit/Core/Extensions/StringExtension.swiftOTPKit/Sources/OTPKit/Core/Helper/Location/LocationManager.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/Itinerary.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/Location.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/TransportMode.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/WalkingDistance.swiftOTPKit/Sources/OTPKit/Core/Services/UserDefaultsServices.swiftOTPKit/Sources/OTPKit/Core/Utilities/Formatters.swiftOTPKit/Sources/OTPKit/Network/RestAPIService.swiftOTPKit/Sources/OTPKit/Presentation/Common/Error/ErrorCardView.swiftOTPKit/Sources/OTPKit/Presentation/Common/Loading/LoadingOverlay.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/AdvancedOptions/AdvancedOptionsSheet.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/AdvancedOptions/Components/OptionRowView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegUnknownView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegVehicleView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegWalkView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/DirectionsSheetView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/ItineraryDetailsView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/PagedDirectionsView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/Components/CurrentLocationButton.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/Components/FavoritesSectionView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/Components/RecentsSectionView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/FavouriteLocationsSheet.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/LocationOptionsSheet.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Library/RecentLocationsSheet.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Search/Components/SearchBar.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Search/SearchSheetView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/TripPlanner/Components/ItineraryLegs/ItineraryLegUnknownView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/TripPlanner/Components/ItineraryPreviewView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/TripPlanner/TripPlannerResultsView.swiftOTPKit/Sources/OTPKit/Presentation/TopControls/TopControlsOverlay.swiftOTPKit/Sources/OTPKit/Presentation/TopControls/TripOptionsSummaryView.swiftOTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swiftOTPKit/Sources/OTPKit/Presentation/ViewModel/TripPlannerViewModel.swiftOTPKit/Sources/OTPKit/Resources/ar.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/en.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/es.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/fil.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/fr.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/it.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/ko.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/pl.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/pt-BR.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/ru.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/vi.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/zh-Hans.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/zh-Hant.lproj/Localizable.stringsOTPKit/Tests/LocalizationTests.swiftREADME.markdown
| /// | ||
| /// OTP sends these as uppercase tokens (`CABLE_CAR`). Rendering the token directly leaks | ||
| /// English-shaped data into every locale, so callers should use ``displayName``. | ||
| public enum LegMode: String, CaseIterable, Sendable { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the new OTP model types Codable.
Both newly introduced model enums omit the repository’s required JSON serialization conformance. Add Codable to both declarations.
OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift#L13-L13: addCodable.OTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swift#L13-L13: addCodable.
As per coding guidelines, “All models must conform to Codable for JSON serialization.”
📍 Affects 2 files
OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift#L13-L13(this comment)OTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swift#L13-L13
🤖 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 `@OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift` at line 13, Make both
model enums conform to Codable: update LegMode in
OTPKit/Sources/OTPKit/Core/Models/OTP/LegMode.swift at lines 13-13 and
RelativeDirection in
OTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swift at lines 13-13,
preserving their existing conformances.
Source: Coding guidelines
| .navigationTitle(destination?.title ?? OTPLoc("map.destination", comment: "The destination of a trip")) | ||
| .toolbarTitleDisplayMode(.inlineLarge) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the unknown-location fallback for the navigation title.
When destination is nil, this resolves to localized "map.destination" instead of the already-computed unknownLocation, making the title inconsistent with the destination description.
Proposed fix
- .navigationTitle(destination?.title ?? OTPLoc("map.destination", comment: "The destination of a trip"))
+ .navigationTitle(destination?.title ?? unknownLocation)📝 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.
| .navigationTitle(destination?.title ?? OTPLoc("map.destination", comment: "The destination of a trip")) | |
| .toolbarTitleDisplayMode(.inlineLarge) | |
| .navigationTitle(destination?.title ?? unknownLocation) | |
| .toolbarTitleDisplayMode(.inlineLarge) |
🤖 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
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/ItineraryDetailsView.swift`
around lines 68 - 69, Update the navigationTitle in ItineraryDetailsView to use
the existing unknownLocation fallback when destination is nil, while preserving
destination?.title for non-nil destinations and removing the separate OTPLoc
fallback.
Four fixes from CodeRabbit's review of #149: - Cached formatters now follow the device. The wire formatters in DateFormatterExtension and the display formatter in Formatters are `static let`/`lazy var`, so a pinned `.current` time zone goes stale the moment the device's changes -- a rider who flies across a boundary keeps planning trips in the departure zone's offset until relaunch. Verified empirically: with TZ flipped from America/Los_Angeles to Asia/Tokyo, a cached formatter pinned to `.current` kept emitting 16:00 while an `.autoupdatingCurrent` one correctly moved to 09:00. The API formatters' locale and calendar stay pinned to en_US_POSIX/Gregorian -- those are deliberate wire-stability guarantees, and only the zone should track. - Show the boarding stop code, not the alighting one. DirectionLegVehicleView printed `leg.to.stopCode` under the departure time, so the boarding row named the stop where the rider gets off. - Drop the dead `errorMessage` state in CurrentLocationButton. It was assigned in two paths and never read; the button communicates failure through its title and subtitle. Removes the two now-orphaned keys from all 13 locale tables (180 -> 178 keys each). - Compare format specifiers by type, not just count. A translation could swap %@ for %d and pass, then crash in String(format:). Comparing raw sequences would instead reject the positional reordering that %1$@/%2$@ exist to permit and that several locales here use, so the check maps argument position to conversion type. Two comments declined: - Making LegMode/RelativeDirection `Codable` would add a second parsing path that disagrees with the real one: the synthesized conformance keys off rawValue alone, so decoding "BIKE" or "TRAIN" would throw where `init?(otpMode:)` resolves them through the alias table. These are presentation adapters over an untyped wire field, not wire models. - The ItineraryDetailsView nav title keeps "Destination" rather than the "Unknown" used for missing places in rows. A title bar reading "Unknown" names nothing; the contexts differ. The formatter pinning test now asserts the zone resolves to the device's *and* is autoupdating -- `.autoupdatingCurrent != .current` even when both resolve identically, so the first check compares identifiers.
|
Worked through the review feedback in aa0532a — four fixed, two declined. Fixed
Declined
165 tests in 13 suites passing, SwiftLint clean. |
1 participant
Summary
Localizable.stringstables (83 → 180 keys per locale, all referenced, none orphaned).LegModeandRelativeDirectionenums map them to localized names, so a French user sees "Tournez à gauche sur Pine St" rather than "LEFT onto Pine St".Defects fixed
Text("Close")in a Swift package never reached the package's tables. SwiftUI'sLocalizedStringKeyinitializers resolve against the main app bundle, so every one of these literals would have missed OTPKit's strings even if translations had existed. All call sites now route throughOTPLoc, which passesbundle: .module.TransportMode.displayNamereferenced four keys that existed in no strings file.NSLocalizedStringechoes the key on failure, so users were seeing the literal texttransport_mode_transitin the UI. Renamed totransport_mode.*, translated into all 13 locales.The OTP API date/time query parameters were locale-fragile.
formattedTripDate/formattedTripTimebuild thedate=andtime=params (RestAPIService.swift:44-45). They are now pinned toen_US_POSIX+ Gregorian + an explicit local time zone. Without the calendar pin, a device on a Buddhist or Japanese Imperial calendar emits05-10-2567; without the zone pin, iOS emits UTC and plans trips for the wrong time of day. Both are covered by tests.Test plan
xcodebuild test— 165 tests in 13 suites pass (was 142)swiftlint— 0 violations across 112 filesInfo.plistchange that language resolution flips from a clean build, and that the generated keys are still merged into the partial plistotp.prod.sound.obaweb.orgusing the new pinned wire format — HTTP 200, 3 itineraries returnedDEPART,LEFT,RIGHT,UTURN_RIGHT,WALK,BUS) are all covered by the new enumsReview notes
Host apps must declare
CFBundleLocalizations. iOS resolves an app's language from the main bundle, so a host app that ships no localizations pins the process to English and OTPKit follows — the translations are present but never selected. Documented in a new README section. The OBA iOS app is already localized into this set, so it is unaffected. The demo app was not, which is why it rendered English;Demo/OTPKitDemo-Info.plistnow declares the 13 locales. That change does not translate the demo's own UI, which stays in English by design.Two Xcode mechanics that cost a build cycle each, worth knowing before editing that plist:
INFOPLIST_KEY_CFBundleLocalizationsis silently ignored (Xcode only passes through an allow-list ofINFOPLIST_KEY_*), and the plist must live outsideDemo/OTPKitDemo/because that is a synchronized folder group — inside it, the file is copied as a resource too and the build fails with "Multiple commands produce ... Info.plist".Unrelated bug, worth its own issue: the demo app crashes on launch under iOS 27 — its generated
UIApplicationSceneManifesthas an emptyUISceneConfigurationsdict, which now hard-traps in_UIApplicationEvaluateRuntimeIssueForNoSceneLifecycleAdoption. Pre-existing; I verified this branch on iOS 26.3.Deliberately deferred, all raised during review and reasonable follow-ups:
leg.modestrings with three different normalizations (DirectionLegView,DirectionLegVehicleView,TripPlannerResultsView,MapCoordinator×3,Leg.walkMode). Migrating them ontoLegModewould unify that, but it changes routing behavior — aTRAINleg would newly reach the vehicle branch — which is out of scope for a localization change.LegModeand the existingRouteTypeare two unbridged taxonomies on the sameLeg."%1$@ onto %2$@, walk %3$@."), which yields "Take the elevator onto Pine St". Fixing properly means per-case whole-sentence keys and wants translator input.WalkingDistancelabels are hardcoded imperial in English and hand-converted to metric elsewhere; unit choice keys off UI language rather than measurement system. Routing throughFormatters.formatDistancewould fix that but rounds 402 m to "0.2 mi", destroying the 0.25/0.5 distinction in the picker.CLAUDE.mdsays package tests use XCTest, but 6 of 7 files inOTPKit/Tests/already use Swift Testing. The doc is stale.Also note the options pill now reads "1 mile walk" where it read "1 mi walk" — slightly wider text in a capsule; no line limit, so it should just grow.