Handle error response returned with HTTP 200

[mosliem]

This PR handles OpenTripPlanner (OTP) error responses that are returned with HTTP 200 OK.

Changes
• Parse OTP error message codes and messages from successful (200) responses
• Introduce ErrorResponseCode enum for type-safe error handling
• Add user-friendly, localized error messages

---

Simulator.Screen.Recording.-.iPhone.16.-.2026-02-09.at.01.30.48.mov

Simulator.Screen.Recording.-.iPhone.16.-.2026-02-09.at.01.30.02.mov

[aaronbrethorst]

Hey Mohamed, thanks for taking the time to work on OTP error handling -- this is a genuinely important improvement. The idea of surfacing specific, actionable error messages instead of silently swallowing HTTP 200 errors is exactly what we need, and the localized error strings you wrote are clear and user-friendly. Before we can merge this, I will need you to make a couple changes:

Critical

1. Verify CodingKeys mapping against a real OTP 1.x server response. The test fixture (otp_response_error.json) has "message": "No trip found that satisfies the requested parameters." -- a human-readable sentence, not a machine-readable error code like "PATH_NOT_FOUND". Your CodingKeys map messageCode to the JSON "message" field, and ErrorResponseCode expects raw values like "PATH_NOT_FOUND". If the fixture is representative of real OTP responses, then messageCode will always decode as .unknown, making the entire 19-case enum effectively dead code. Please verify against an actual OTP server: does the "message" JSON field contain codes like "PATH_NOT_FOUND", or human-readable strings? If OTP sends codes there, update the fixture. If it sends human-readable text, the enum may need to be keyed off the id field instead.

2. Update the test fixture and add assertions for messageCode. The test at RestAPIServiceComprehensiveTests.swift:316 only checks response.error?.message but never asserts on response.error?.messageCode. Update the fixture's "message" field to contain a real OTP error code (e.g., "PATH_NOT_FOUND"), and add: #expect(response.error?.messageCode == .pathNotFound). Also add a separate test for the .unknown fallback (where the "message" field contains an unrecognized string).

3. Add a ViewModel-level test for the new error path. The handlePlanResponse error branch (when response.error is non-nil) is the centerpiece of this PR, but it has zero test coverage. TripPlannerViewModelTests only tests the happy path. Add a test that verifies: showingError == true, errorMessage contains the expected localized message, and isLoading == false when the API returns an error within an HTTP 200 response.

Important

4. Don't store the error response in tripPlanResponse. In handlePlanResponse (TripPlannerViewModel.swift:250-261), you unconditionally set tripPlanResponse = response before checking for errors. When there's an error, this overwrites any previously displayed trip results with a response that has plan: nil, causing results to silently disappear alongside the error alert. Move the assignment into the success branch, or explicitly set tripPlanResponse = nil on the error branch to make the intent clear:

   if let error = response.error {
       tripPlanResponse = nil
       // ... show error
   } else {
       tripPlanResponse = response
       // ... success
   }

5. Log the original value when falling back to .unknown. In ErrorResponseCode.init(from decoder:) (ErrorResponse.swift:65-70), unrecognized error codes are silently discarded. If an OTP server sends a new code your client doesn't know about, there's no trace of what it was. Add logging:

   if let known = ErrorResponseCode(rawValue: value) {
       self = known
   } else {
       Logger.main.warning("Unrecognized OTP error code: \(value)")
       self = .unknown
   }

Fit and Finish

6. Fix the double space in the doc comment. ErrorResponse.swift:29 -- "A message key" has an extra space.

7. Remove the redundant isLoading = false. In handlePlanResponse (line 251), you set isLoading = false at the top, but showError also sets it on line 272. Either remove it from handlePlanResponse and let each branch manage it, or remove it from showError and set it once at the top. Having it in both places is confusing for maintainers.

8. Consider an exhaustive switch in displayMessage. The default branch that dynamically constructs localization keys ("error.\(rawValue.lowercased())") is convenient but fragile -- adding a new enum case won't trigger a compiler warning if the corresponding .strings entry is missing. An exhaustive switch would let the compiler enforce that every case has a localization mapping.

Thanks again, and I look forward to merging this change!

[aaronbrethorst]

Hey Mohamed, you've done a really thorough job addressing every piece of feedback from the first review. The CodingKeys mapping is now correct, the test fixtures match real OTP server responses, the ViewModel error path has solid coverage, and the exhaustive switch on displayMessage is exactly what I was hoping for. The way you restructured handlePlanResponse to cleanly separate the error and success branches -- including nil-ing out tripPlanResponse and removing the redundancy in isLoading management -- shows careful attention to detail.

There's nothing left to change -- this PR is ready to merge. Really nice contribution to OTPKit's error handling; users are going to see much more helpful messages when trip planning hits a snag.