Surveys Feature UI

[mosliem]

This PR implements the Surveys UI, covering the full survey flow and its integration across the app.

• Survey question views (full-form layout)
• SurveysViewModel for flow orchestration
• Integration with StopViewController and MapViewController
• Toast messages for validation and feedback

---

Screenshots

---

Related to

• [Surveys] Design UI for Survey View #820
• [GSoC` 25] - Survey Feature #819
• [Surveys] - Add External Survey Support #831

[aaronbrethorst]

Thank you for this substantial contribution. The Surveys Feature UI is well-structured and demonstrates strong understanding of both SwiftUI and UIKit integration patterns. The use of iOS 26's Liquid Glass API is correct, and the withObservationTracking pattern follows Apple's documented approach for UIKit observation.

Below is the feedback organized by priority. All items require attention before or shortly after merge.

---

Priority 1: Must Fix Before Merge

1.1 Constraint Conflict in MapViewController

File: OBAKit/Mapping/MapViewController.swift (lines 99-104)

The survey popup constraints are unsatisfiable. You've set leading, trailing, AND a fixed width constraint:

NSLayoutConstraint.activate([
    hostingController.view.trailingAnchor.constraint(equalTo: view.safeAreaLayoutGuide.trailingAnchor, constant: -ThemeMetrics.controllerMargin),
    hostingController.view.leadingAnchor.constraint(equalTo: view.safeAreaLayoutGuide.leadingAnchor, constant: ThemeMetrics.controllerMargin),
    hostingController.view.widthAnchor.constraint(equalToConstant: view.bounds.width)  // Remove this line
])

Action: Remove the width constraint. The leading and trailing constraints already define the width.

---

1.2 Missing Child View Controller Lifecycle in HeroQuestionCell

File: OBAKit/Surveys/View/Components /HeroQuestionCell.swift (lines 758-773)

The UIHostingController is created but not retained or managed as a child view controller. This will cause the hosting controller to deallocate immediately after setupView() returns, leading to undefined behavior or crashes.

private func setupView() {
    let swiftUIView = getQuestionView()
    let hostingController = UIHostingController(rootView: swiftUIView)
    // hostingController is a local variable - it will be deallocated
    contentView.addSubview(hostingController.view)
}

Action: Store the hosting controller as an instance property. If the cell needs to be a proper container, call addChild() and didMove(toParent:) on the owning view controller, or restructure to avoid embedding UIHostingController in a collection view cell.

---

1.3 Same Issue in MapViewController

File: OBAKit/Mapping/MapViewController.swift (lines 109-118)

The surveyPopupController is stored (good), but addChild() and didMove(toParent:) are never called. Similarly, dismissSurveyPopup() removes the view but doesn't call willMove(toParent: nil) and removeFromParent().

Action: Add proper child view controller lifecycle calls:

private func showSurveyHeroQuestionPopup() {
    // ... create hostingController ...
    addChild(hostingController)
    view.insertSubview(hostingController.view, aboveSubview: mapRegionManager.mapView)
    // ... constraints ...
    hostingController.didMove(toParent: self)
    surveyPopupController = hostingController
}

@objc private func dismissSurveyPopup() {
    surveyPopupController?.willMove(toParent: nil)
    surveyPopupController?.view.removeFromSuperview()
    surveyPopupController?.removeFromParent()
    surveyPopupController = nil
}

---

1.4 Directory Name Contains Trailing Space

Path: OBAKit/Surveys/View/Components /

The directory name has a trailing space. This will cause issues with build systems, git operations, and cross-platform development.

Action: Rename to Components (no trailing space).

---

1.5 Add @mainactor to SurveysViewModel

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift

The @Observable class updates UI-bound state from Task blocks but isn't marked @MainActor. This can cause data races when observable properties are modified from background threads.

Action: Add @MainActor to the class declaration:

@Observable
@MainActor
final public class SurveysViewModel { ... }

---

Priority 2: Must Fix Shortly After Merge

2.1 Silent Error Swallowing in fetchSurveys()

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift (lines 1699-1710)

Network errors are caught and discarded with only print(). Users see no survey and receive no indication why.

} catch {
    print(error)  // Silent failure
}

Action:

1. Replace print(error) with Logger.error("Failed to fetch surveys: \(error)")
2. Either set self.error = error to trigger error observation, or show a toast indicating surveys couldn't be loaded

---

2.2 SurveyService Returns Silently on Nil apiService

File: OBAKitCore/Surveys/Service/SurveyService.swift

When apiService is nil, methods log an error but return without throwing. Callers assume the operation succeeded.

guard let apiService else {
    Logger.error("Survey API service is nil.")
    return  // Callers think this succeeded
}

Action: Throw an error instead of returning silently. Define a SurveyError.serviceUnavailable case if needed.

---

2.3 Make ViewModel State Properties Private(set)

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift

Public mutable properties allow external code to bypass the onAction() state machine:

public var showToastMessage: Bool = false
public var showFullSurveyQuestions: Bool = false
public var showSurveyDismissSheet: Bool = false

Action: Change to private(set) public var for all state properties that should only be modified internally.

---

2.4 HeroQuestionListItem Should Use Stable ID

File: OBAKit/Surveys/View/Components /HeroQuestionCell.swift

Using UUID().uuidString for the item ID means every reconstruction creates a "new" item, causing unnecessary diffing and cell reloads:

init(...) {
    self.id = UUID().uuidString  // Unstable identity
}

Action: Use the question's ID for stable identity:

var id: Int { question.id }

Update hash(into:) and == accordingly.

---

2.5 Clear Error State After Display

File: OBAKit/Surveys/Protocol/SurveyViewHostingProtocol.swift (lines 462-475)

When showSurveyError() is called, surveysVM.error is never cleared. If observation re-triggers, the same error may display again.

Action: Clear the error after displaying:

func observeSurveyError() {
    withObservationTracking { [weak self] in
        if let error = self?.surveysVM.error {
            self?.showSurveyError(error)
            self?.surveysVM.error = nil  // Clear after showing
        }
    } onChange: { ... }
}

---

2.6 Standardize Error Handling Pattern

Files: SurveysViewModel.swift

Error handling is inconsistent across methods:

Method	Logs	User Feedback	Sets error
fetchSurveys()	print()	None	No
submitHeroQuestionAnswer()	None	Via observation	Yes
submitSurveyQuestionsAnswers()	print()	Toast	No

Action: Establish a consistent pattern:

1. Always log with Logger.error()
2. Always set self.error for observation-based display, OR always use toast—pick one approach and apply it consistently

---

Priority 3: Address in Follow-up

3.1 Reduce Code Duplication Between Controllers

Files: MapViewController.swift, StopViewController.swift

Approximately 160 lines are duplicated between these files for survey integration. The SurveyViewHostingProtocol provides some defaults but presentFullSurveyQuestions(), openSafari(with:), and observeSurveyDismissActionSheet() are copy-pasted.

Action: Move these implementations into the protocol extension, or extract a shared coordinator.

---

3.2 Consolidate Toast State

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift

Toast state is fragmented across three properties:

public var showToastMessage: Bool = false
public var toastMessage: String = ""
public var toastType: Toast.ToastType = .success

Action: Consolidate to a single optional:

public var activeToast: Toast?

---

3.3 Remove Unused Property

File: OBAKitCore/Surveys/Helper/ExternalSurveyURLBuilder.swift (line 2431)

The stop instance property is declared but never assigned. The buildURL() method takes stop as a parameter instead.

Action: Remove the unused private var stop: Stop? property.

---

3.4 Fix Typo

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift (line 1585)

/// Dependancies  →  /// Dependencies

---

Summary

This is solid work. The architecture is sound, the iOS 26 Liquid Glass integration is correct, and the observation patterns follow Apple's guidance. The critical issues are straightforward to fix—primarily around view controller lifecycle management and Auto Layout constraints.

Please address Priority 1 items before merge, and Priority 2 items in a fast-follow PR.

[mosliem]

@aaronbrethorst
Thanks for the review!
I’ve resolved all comments except

2.3. Some of the state properties can’t be marked private(set)

At the moment, because they’re mutated via @Binding in SurveyQuestionsForm for the toast modifier.

[aaronbrethorst]

Hi Mohamed,

This is looking really good. The architecture is solid: using SurveyViewHostingProtocol with default implementations keeps the integration code in StopViewController and MapViewController clean, and the action-based pattern in SurveysViewModel makes the flow easy to follow. We're getting close, but there are a few more items that need attention before merging.

---

Critical (Please Fix)

1. Task.sleep error swallowed in clearExternalSurveyState()

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift:285-293

Task { @MainActor [weak self] in
    try await Task.sleep(for: .seconds(2))  // Can throw CancellationError
    self?.heroQuestion = nil                 // Never executes if sleep throws
    // ...
}

If the task is cancelled, the cleanup never happens and the external survey state remains stale. Either use try? or wrap in do-catch and perform cleanup regardless:

Task { @MainActor [weak self] in
    try? await Task.sleep(for: .seconds(2))
    self?.heroQuestion = nil
    // ...
}

2. ExternalSurveyURLBuilder has unused stop property

File: OBAKitCore/Surveys/Helper/ExternalSurveyURLBuilder.swift:19

private var stop: Stop?  // Never assigned - shadows the parameter

This property is declared but never set. The buildURL method correctly uses its parameter, but this instance property is dead code. Please remove it.

---

Important (Please Address)

3. buildURL() fails silently - user taps "Go" and nothing happens

File: OBAKitCore/Surveys/Helper/ExternalSurveyURLBuilder.swift:27-42

When URL building fails (missing base URL, invalid URL), the method returns nil and handleOpenExternalSurvey() silently returns. The user gets no feedback. Add logging and consider showing an error toast:

guard let baseURL = survey.questions.first?.content.url else {
    Logger.error("External survey URL missing for survey ID: \(survey.id)")
    return nil
}

4. ToastManager singleton breaks dependency injection pattern (this can be deferred til later)

File: OBAKit/ToastMessageBar/ToastManager.swift:13-14

The codebase uses DI through the Application class, but ToastManager.shared is a singleton. This makes testing harder and doesn't match the established pattern. Consider injecting it through the existing infrastructure. If you choose not to fix this now, please open an issue tracking it.

5. Inconsistent concurrency patterns

File: OBAKit/Surveys/ViewModel/SurveysViewModel.swift:256-261

Uses DispatchQueue.main.asyncAfter here but Task.sleep elsewhere (line 287). Pick one approach for consistency. Since you're using Swift Concurrency elsewhere, Task.sleep is preferred.

6. Empty surveyPathId could cause API errors

File: OBAKitCore/Surveys/Service/SurveyService.swift:117

let surveyResponseId = surveyStore.getSurveyResponse()?.surveyPathId() ?? ""

Using an empty string fallback will likely cause a 404/400. Validate before the API call and throw a meaningful error.

---

Suggestions (Take or Leave)

7. Generic constraint on MapHeroQuestionView is unnecessary

File: OBAKit/Surveys/View/Components/MapHeroQuestionView.swift:11-14

SurveysViewModel is a final class, not a protocol. The generic <ViewModel: SurveysViewModel> adds no value. Simplify to let viewModel: SurveysViewModel.

8. Consider adding ExternalSurveyURLBuilder tests

This class embeds user data (user_id, region_id, stop_id, current_location) into URLs. URL construction bugs could cause silent data loss or privacy issues. It's pure logic with no UI dependencies - easy to test in OBAKitCoreTests.

9. Minor SwiftLint warnings

• ToastView+UIViewController.swift:10 - Extra blank line
• SurveyQuestionAnswer.swift:10 - Extra blank line

---

What's Done Well

• Protocol with defaults: SurveyViewHostingProtocol keeps integration code minimal in both view controllers
• Action-based state management: SurveysAction enum makes flows traceable
• Memory management: Proper [weak self] throughout async closures
• Accessibility: SwiftUI components include proper labels
• Localization: New strings properly added to Strings.swift and Localizable.strings
• Existing test coverage: Service, prioritizer, and state manager are well-tested at 30+ test cases

---

Please fix the critical items (Task.sleep error handling, unused stop property) and address the silent URL building failure. The rest are suggestions you can take or leave.

Once those are addressed, this is a solid implementation ready to ship! 🚀

[aaronbrethorst]

Hey Mohamed, great progress on the Surveys UI -- you've addressed all the feedback from the previous two rounds, and the architecture with SurveyViewHostingProtocol and the action-based SurveysViewModel is really well thought out. A few more issues turned up on this pass, so here's what needs attention before we merge:

Critical

1. Duplicate observation chains cause double side-effects

Files: MapViewController.swift:134,154 and StopViewController.swift:216,232

observeSurveysState() is called in both viewDidLoad and viewWillAppear. Since viewWillAppear always runs after viewDidLoad on initial presentation (before any viewWillDisappear), this creates two parallel observation chains for all 6 observers (12 active loops instead of 6). When a property changes, both chains fire — showing duplicate toasts, calling showSurveyHeroQuestionPopup() twice, calling listView.applyData() twice, etc.

Fix: Remove the observeSurveysState() call from viewDidLoad in both controllers. The viewWillAppear call is sufficient and correctly re-registers observation on each appearance:

// viewDidLoad:
surveysVM.onAction(.onAppear)
// observeSurveysState()  ← remove this line

// viewWillAppear (unchanged):
observeSurveysState()

2. setHeroQuestion sets showHeroQuestion = true even when no hero question exists

File: SurveysViewModel.swift:182-187

If a survey contains only label-type questions, .first { $0.content.type != .label } returns nil. heroQuestion is set to nil, but showHeroQuestion is unconditionally set to true. On the map screen, this adds an invisible empty popup as a child view controller. On the stop screen, the survey section returns nil but the ViewModel is in an inconsistent state.

private func setHeroQuestion() {
    guard let survey else { return }
    let firstQuestion = survey.getQuestions().first { $0.content.type != .label }
    self.heroQuestion = firstQuestion       // Could be nil
    self.showHeroQuestion = true            // Always true — even when heroQuestion is nil
}

Fix: Only show if a question was found:

private func setHeroQuestion() {
    guard let survey else { return }
    guard let firstQuestion = survey.getQuestions().first(where: { $0.content.type != .label }) else { return }
    self.heroQuestion = firstQuestion
    self.showHeroQuestion = true
}

3. Array index out of bounds risk in getNextSurvey

File: SurveysViewModel.swift:159-165

prioritizer.nextSurveyIndex() returns an Int with -1 as a sentinel for "not found." The code checks != -1 but doesn't validate the index is within bounds. If the surveys array changes between the prioritizer call and the subscript access, this crashes.

private func getNextSurvey() {
    let surveyIndex = prioritizer.nextSurveyIndex(service.surveys, visibleOnStop: stopContext, stop: stop)
    if surveyIndex != -1 {
        self.survey = service.surveys[surveyIndex]  // CRASH if index >= surveys.count
        setHeroQuestion()
    }
}

Fix: Capture the array and add bounds checking:

private func getNextSurvey() {
    let surveys = service.surveys
    let surveyIndex = prioritizer.nextSurveyIndex(surveys, visibleOnStop: stopContext, stop: stop)
    guard surveyIndex >= 0, surveyIndex < surveys.count else { return }
    self.survey = surveys[surveyIndex]
    setHeroQuestion()
}

Important

4. Hardcoded English accessibility strings

Files: SelectionQuestionView.swift:118-123, TextQuestionView.swift:33-34, SurveyLabelView.swift:28

Several accessibility strings are hardcoded in English rather than using OBALoc/Strings.*. VoiceOver reads these aloud to users, so they must be localized:

• "Selected" / "Not selected" / "Tap to toggle selection" / "Tap to select" in SelectionQuestionView
• "Answer input" / "Enter your answer to the survey question" in TextQuestionView
• "Survey Label" in SurveyLabelView

Fix: Add these strings to Strings.swift and Localizable.strings, and reference them via Strings.*.

5. surveyPathId() returns empty string instead of nil, bypassing the guard in SurveyService

File: SurveySubmission.swift:99-101

surveyPathId() returns String, not String?. If updatePath is empty or malformed (e.g., "/"), it returns "". The caller in SurveyService.updateSurveyResponses uses guard let surveyResponseId = surveyStore.getSurveyResponse()?.surveyPathId() — but this only checks whether getSurveyResponse() is nil, not whether the path ID is empty. An empty string passes the guard and gets sent to the API, resulting in a server error.

Fix: Either change the return type to String?:

public func surveyPathId() -> String? {
    guard let pathId = updatePath.split(separator: "/").last.map(String.init),
          !pathId.isEmpty else {
        return nil
    }
    return pathId
}

Or add an emptiness check in the caller.

6. study property is never assigned — dead code in SurveyQuestionsForm

File: SurveysViewModel.swift:36, SurveyQuestionsForm.swift:54-69

SurveysViewModel.study is declared but never assigned anywhere in the view model. It's always nil, which means the surveyStudyInfo view in SurveyQuestionsForm never renders — the study name/description header is permanently invisible.

Fix: Either populate study from the fetched survey data (e.g., in getNextSurvey()), or remove both the property and the surveyStudyInfo view until the data pipeline supports it.

7. HeroQuestionListItem equality ignores answer — diffable data source won't update

File: HeroQuestionCell.swift:69-75

The Hashable/Equatable implementations only use id. When the user updates their hero question answer, the diffable data source sees no change and won't reconfigure the cell:

func hash(into hasher: inout Hasher) {
    hasher.combine(id)     // answer not included
}

Fix: Include answer in hashing and equality:

func hash(into hasher: inout Hasher) {
    hasher.combine(id)
    hasher.combine(answer)
}

static func == (lhs: HeroQuestionListItem, rhs: HeroQuestionListItem) -> Bool {
    return lhs.id == rhs.id && lhs.answer == rhs.answer
}

Fit and Finish

8. Localizable.strings inconsistency for survey_success.submitted

Files: Strings.swift, en.lproj/Localizable.strings

The Strings.swift value ends with a period, the Localizable.strings value does not. Also, "please" should be capitalized as it starts a new sentence:

"Survey answer submitted! Please answer the rest of the questions for better feedback."

9. showToastMessage naming collision — property and method share the same name

File: SurveysViewModel.swift:23,167

The public property showToastMessage: Bool and the private method showToastMessage(_:type:) share the same name. Inside the method, showToastMessage = true looks like a recursive call at first glance. Rename the method to displayToast(_:type:) or rename the property to isToastVisible.

10. getRecentStopIds() returns empty string instead of nil

File: ExternalSurveyURLBuilder.swift:88-90

joined(separator:) on an empty array returns "", so the recent_stop_ids= query parameter is always added — even when empty. Other helpers (getRegionID, getRouteId) properly return nil to omit the parameter.

Fix:

private func getRecentStopIds() -> String? {
    let ids = userStore.recentStops.map { $0.id }.joined(separator: ",")
    return ids.isEmpty ? nil : ids
}

---

Please address the Critical and Important items and push an update. The Fit and Finish items can be handled in this PR or a fast-follow. Thanks for your persistence through three rounds of review — the feature is looking really solid.

[aaronbrethorst]

Hey Mohamed, the survey feature is really coming together and you've addressed all the issues from the previous three rounds -- great job on that. Before we can merge this, I need you to make a few more changes:

Critical

1. Label-type questions are submitted to the API with empty answers (SurveysViewModel.swift:313, 338-348)

   In setSurveysQuestions(), all questions -- including labels -- are added to questionsAnswers with .text(""). Then buildQuestionsAnswersModel() includes every entry in questionsAnswers in the API submission, sending QuestionAnswerSubmission entries with questionType: "label" and an empty answer string to the server. Labels are informational text, not answerable questions.

   Fix: Filter out label questions when initializing questionsAnswers:

   questions
       .filter { $0.content.type != .label }
       .forEach { self.questionsAnswers[$0.id] = .text("") }

   Also guard against a form with only labels (no answerable questions) by checking answerableQuestionCount > 0 before showing the full survey form. Without that check, ProgressView gets total: 0.0, and the user sees a "0 / 0" progress bar with an immediately submittable empty form.

2. isLoading gets stuck true when Task guard fails (SurveysViewModel.swift:217-220, 354-357)

   In both submitHeroQuestionAnswer() and submitSurveyQuestionsAnswers(), isLoading = true is set before the Task is created. If the [weak self] guard inside the Task fails (e.g., the view model is deallocated), the defer { isLoading = false } never executes. The user sees a permanent loading spinner and all form interactions are disabled.

   Fix: Move isLoading = true inside the Task, after the guard succeeds:

   Task { [weak self] in
       guard let self, let survey else { return }
       self.isLoading = true
       defer { self.isLoading = false }
       // ...
   }

3. fatalError() with no message in production code (HeroQuestionCell.swift:20)

   If apply(_:) receives an unexpected configuration type, the app crashes with no diagnostic information. Use assertionFailure (debug-only) with a graceful return instead:

   guard let config = config as? HeroQuestionContentConfiguration else {
       assertionFailure("HeroQuestionCell received unexpected config: \(type(of: config))")
       return
   }

Important

4. Race condition in observeSurveyToastMessage() (SurveyViewHostingProtocol.swift:77-97)

   The method reads surveysVM.toast?.type to decide the toast type, then reads surveysVM.toast?.message separately in the switch body. If the toast is cleared between these two reads, the message is nil and showErrorToast(nil) hits a guard and silently drops the error.

   Fix: Capture the toast once:

   guard let self, self.surveysVM.showToastMessage, let toast = self.surveysVM.toast else { return }
   switch toast.type {
   case .error: showErrorToast(toast.message)
   case .success: showSuccessToast(toast.message)
   }

5. Redundant do { try ... } catch { throw error } blocks (SurveyService.swift:43-48, 87-92, 136-143)

   All three API methods have do/catch blocks that simply rethrow the error unchanged. Since the methods are already throws, remove the wrappers and let errors propagate naturally. This removes ~18 lines of unnecessary code.

6. No tests for 2,234 lines of new code

   The PR adds a complete UI layer with no test coverage. The existing 30+ tests cover the lower layers (prioritizer, API service, state manager), but the new SurveysViewModel, ExternalSurveyURLBuilder, and SurveyQuestionAnswer.stringValue are untested. The view model is highly testable -- all four dependencies are protocol-based and mocks already exist for SurveyPreferencesStore. I'd like to see at minimum:

   • SurveysViewModel state machine tests (hero question flow, validation, skip/postpone)
   • ExternalSurveyURLBuilder URL construction tests

Fit and Finish

7. Sentinel value -1 for hero question ID (SurveysViewModel.swift:247) -- Use heroQuestion?.id (an Optional<Int>) and if let heroQuestionID instead of ?? -1.

8. Use switch instead of if/else if chain (SurveyQuestionView.swift:83-95) -- The questionContent property uses if case .text = type / else if case .radio / else if case .checkbox. A switch makes exhaustive handling explicit and removes the duplicated question.content.options ?? [] line.

9. Use if let in surveyStudyInfo (SurveyQuestionsForm.swift:52-70) -- The view checks if viewModel.study != nil but then uses optional chaining viewModel.study?.name ?? "" inside the body. Use if let study = viewModel.study and access study.name directly.

10. Deduplicate submitButtonLabel (SurveyQuestionsForm.swift:131-157) -- The iOS 26 and pre-iOS 26 branches share identical text styling. Extract the shared Text(Strings.submit) styling and only branch for the .glassEffect vs .background modifier.

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

[aaronbrethorst]

Hey Mohamed, you've done excellent work addressing the feedback from the previous four rounds -- the @MainActor annotation, private(set) properties, bounds checking, label filtering, throws refactoring, fatalError replacement, and the test suite are all solid. The SurveysViewModelTests and ExternalSurveyURLBuilderTests you added are well-structured with good mock design and async wait helpers.

I have elected to retarget this at a feature branch so we can get away from the massive PR and start working on incremental changes instead. I am merging this PR, but I want you to follow up with changes for the following items.

Critical

1. showHeroQuestion not reset on external survey path -- hero popup stays visible on map

File: SurveysViewModel.swift -- handleOpenExternalSurvey() and clearExternalSurveyState()

When the user taps "Go" on an external survey, handleOpenExternalSurvey() sets openExternalSurvey = true and marks the survey completed, but never sets showHeroQuestion = false. The clearExternalSurveyState() sets heroQuestion = nil after a 2-second delay, but showHeroQuestion remains true. On the map screen, observeSurveyHeroQuestion() checks showHeroQuestion to decide whether to show/hide the popup, so the popup hosting controller stays in the view hierarchy.

Compare with setRemainingSurveyQuestions() which correctly sets self.showHeroQuestion = false, and clearHeroQuestionState() which also does this.

Fix: Add showHeroQuestion = false in clearExternalSurveyState(), or call clearHeroQuestionState() from handleOpenExternalSurvey():

private func clearExternalSurveyState() {
    Task { @MainActor [weak self] in
        try? await Task.sleep(for: .seconds(2))
        self?.showHeroQuestion = false   // Add this
        self?.heroQuestion = nil
        self?.externalSurveyURL = nil
        self?.openExternalSurvey = false
        self?.removeSurvey()
    }
}

2. observeSurveyLoadingState() missing [weak self] -- ProgressHUD can get stuck indefinitely

File: SurveyViewHostingProtocol.swift -- observeSurveyLoadingState()

Unlike all the other observe* methods in this protocol, the withObservationTracking closure does not use [weak self]:

func observeSurveyLoadingState() {
    withObservationTracking {
        if surveysVM.isLoading {       // No [weak self] guard
            ProgressHUD.show()
        } else {
            ProgressHUD.dismiss()
        }
    } onChange: { ... }
}

If isLoading becomes true and the view controller is dismissed before it returns to false, the observation loop stops (via observationActive), but ProgressHUD.dismiss() is never called. The ProgressHUD stays visible indefinitely, blocking the UI.

Fix: Add [weak self] and dismiss the HUD when observation stops:

func observeSurveyLoadingState() {
    withObservationTracking { [weak self] in
        guard let self else { return }
        if self.surveysVM.isLoading {
            ProgressHUD.show()
        } else {
            ProgressHUD.dismiss()
        }
    } onChange: {
        Task { @MainActor [weak self] in
            guard let self, self.observationActive else { return }
            self.observeSurveyLoadingState()
        }
    }
}

func stopObserveSurveysState() {
    observationActive = false
    ProgressHUD.dismiss()  // Ensure HUD is dismissed when observation stops
}

3. Wrong accessibility hint in TextQuestionView

File: TextQuestionView.swift:15

The accessibility hint uses the validation error message instead of the actual hint:

.accessibilityHint(Strings.surveyHeroQuestionAnswerError)
// Shows: "Please enter a valid answer" -- this is an error message, not a hint

You already have the correct string defined: Strings.answerInputQuestionAccessibility ("Enter your answer to the survey question").

Fix:

.accessibilityHint(Strings.answerInputQuestionAccessibility)

Important

4. observeSurveyToastMessage reads toast twice -- race condition from round 4

File: SurveyViewHostingProtocol.swift -- observeSurveyToastMessage()

This was flagged in the previous review (round 4, item 4) and still needs to be addressed. The guard extracts toast?.type, then the switch reads surveysVM.toast?.message separately. If toast is cleared between these two reads, the message is nil and the toast is silently dropped.

Fix: Capture the toast once:

func observeSurveyToastMessage() {
    withObservationTracking { [weak self] in
        guard let self,
              let toast = self.surveysVM.toast,
              self.surveysVM.showToastMessage
        else { return }

        switch toast.type {
        case .error:
            showErrorToast(toast.message)
        case .success:
            showSuccessToast(toast.message)
        }
    } onChange: { /* unchanged */ }
}

5. CoreApplication.externalSurveyURLBuilder uses concrete type instead of protocol

File: CoreApplication.swift:239

The other survey dependencies use protocol types (surveyStateManager: SurveyStateProtocol, surveyPrioritizer: SurveyPrioritizing), but this one uses the concrete type:

public lazy var externalSurveyURLBuilder: ExternalSurveyURLBuilder = ExternalSurveyURLBuilder(...)

Fix: Use the protocol type:

public lazy var externalSurveyURLBuilder: ExternalSurveyURLBuilderProtocol = ExternalSurveyURLBuilder(...)

6. Close button double-dismisses SurveyQuestionsForm

File: SurveyQuestionsForm.swift -- closeButton

The close button calls both viewModel.onAction(.onCloseQuestionsForm) (which sets showFullSurveyQuestions = false) and dismiss(). But the onChange(of: viewModel.showFullSurveyQuestions) modifier also calls dismiss() when it detects false. So tapping the close button triggers dismiss() twice.

Button {
    viewModel.onAction(.onCloseQuestionsForm)  // Sets showFullSurveyQuestions = false
    dismiss()                                    // First dismiss
} label: { ... }

// Then onChange fires:
.onChange(of: viewModel.showFullSurveyQuestions) { _, isShown in
    if isShown == false {
        dismiss()  // Second dismiss
    }
}

Fix: Remove the explicit dismiss() from the close button since onChange handles it:

Button {
    viewModel.onAction(.onCloseQuestionsForm)
} label: { ... }

7. UIColor.toColor() extension is inconsistent with existing codebase

File: UIKitExtensions.swift

The PR adds UIColor.toColor() -> Color, but the rest of the codebase already uses Color(uiColor:) directly in 13+ locations (MapItemView.swift, OnboardingHeaderView.swift, RefreshButton.swift, etc.). This introduces a second pattern for the same operation.

Fix: Remove the extension and use Color(uiColor:) throughout the new survey views to match existing conventions. For example:

// Instead of:
ThemeColors.shared.brand.toColor()
// Use:
Color(uiColor: ThemeColors.shared.brand)

Fit and Finish

8. SurveyError is defined in the wrong file

File: NetworkOperation.swift

SurveyError is a survey-domain error but lives in NetworkOperation.swift alongside the unrelated APIError. Move it to its own file under OBAKitCore/Surveys/.

9. Missing space in MARK comment

File: CoreApplication.swift

//MARK: - Survey URL ApplicationContext

Should be:

// MARK: - Survey URL ApplicationContext

10. SurveyQuestionView helper extension should be private

File: SurveyQuestionView.swift

The textAnswerValue and selectionAnswerValues computed properties in the extension are internal by default but are only used within the view's own questionContent. Mark them private.

terrific work, thank you for sticking with this!