Replace marker tutorial storyboard with SwiftUI - #297
Open
RDMurray wants to merge 1 commit into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe marker tutorial was migrated from storyboard/UIKit outlet rendering to a SwiftUI view backed by Marker tutorial migration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
RDMurray
marked this pull request as ready for review
July 28, 2026 13:16
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/GuideDogs/Code/Visual UI/View Controllers/Tutorials/Markers/MarkerTutorialViewController.swift (1)
285-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnnecessary
DispatchQueue.main.asyncaroundviewState.show(...)races the accessibility notification.
show(page:)is only ever reached from main-thread contexts (viewWillAppear, or completions already dispatched viaDispatchQueue.main.async/asyncAfter), so wrappingviewState.show(...)in another.main.asyncjust defers the state write by a run-loop tick. TheUIAccessibility.post(...)call right after (and thecurrentPageIndex/page.buttonTitlechecks) run synchronously before that queued closure executes, so the layout-changed notification fires — andself.currentPageIndexis captured inside the closure — before the state update it's supposed to reflect has actually applied. TheviewState.updateText(nil)call later in the same method (line 322) is invoked directly, showing the direct call is already safe here.🐛 Proposed fix: call directly instead of deferring via async
- DispatchQueue.main.async { - self.viewState.show(title: page.title, - image: page.image, - text: nil, - actionTitle: page.buttonTitle, - action: page.action, - hidesContentFromAccessibility: self.currentPageIndex != UInt(PageIndexes.intro)) - } + viewState.show(title: page.title, + image: page.image, + text: nil, + actionTitle: page.buttonTitle, + action: page.action, + hidesContentFromAccessibility: currentPageIndex != UInt(PageIndexes.intro))🤖 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 `@apps/ios/GuideDogs/Code/Visual` UI/View Controllers/Tutorials/Markers/MarkerTutorialViewController.swift around lines 285 - 306, In show(page:), call viewState.show(...) directly instead of wrapping it in DispatchQueue.main.async, so the state update completes before the subsequent accessibility notification and page checks. Preserve the existing show arguments and leave the later revealAction dispatch unchanged.
🧹 Nitpick comments (1)
apps/ios/GuideDogs/Code/Visual UI/View Controllers/Tutorials/Markers/MarkerTutorialViewController.swift (1)
138-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBackground color literal duplicated across files.
(36/255, 58/255, 102/255)is defined as a SwiftUIColorinMarkerTutorialView.swift(lines 82-84) and re-declared here asUIColorfor the container view. A shared constant (e.g. aUIColorextension consumed by both, or exposing the SwiftUIColor's components) would prevent the two from drifting apart if the palette changes.♻️ Proposed shared constant
extension UIColor { static let markerTutorialBackground = UIColor(red: 36.0 / 255.0, green: 58.0 / 255.0, blue: 102.0 / 255.0, alpha: 1) }- view.backgroundColor = UIColor(red: 36.0 / 255.0, - green: 58.0 / 255.0, - blue: 102.0 / 255.0, - alpha: 1) + view.backgroundColor = .markerTutorialBackgroundAnd in
MarkerTutorialView.swift:- private let backgroundColor = Color(red: 36.0 / 255.0, - green: 58.0 / 255.0, - blue: 102.0 / 255.0) + private let backgroundColor = Color(uiColor: .markerTutorialBackground)🤖 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 `@apps/ios/GuideDogs/Code/Visual` UI/View Controllers/Tutorials/Markers/MarkerTutorialViewController.swift around lines 138 - 141, Replace the duplicated background color literal in MarkerTutorialViewController with a shared marker tutorial background constant, such as UIColor.markerTutorialBackground. Update MarkerTutorialView to consume the same shared palette value so both UIKit and SwiftUI views remain synchronized when the color changes.
🤖 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 `@apps/ios/UnitTests/MarkerTutorialViewStateTests.swift`:
- Around line 94-97: In apps/ios/UnitTests/MarkerTutorialViewStateTests.swift
lines 94-97, capture the original AppContext.shared.isInTutorialMode value
before the test and restore that captured value in the defer block instead of
forcing false; at lines 145-147, capture the scene’s previous key window before
makeKeyAndVisible() and restore it as key during cleanup.
---
Outside diff comments:
In `@apps/ios/GuideDogs/Code/Visual` UI/View
Controllers/Tutorials/Markers/MarkerTutorialViewController.swift:
- Around line 285-306: In show(page:), call viewState.show(...) directly instead
of wrapping it in DispatchQueue.main.async, so the state update completes before
the subsequent accessibility notification and page checks. Preserve the existing
show arguments and leave the later revealAction dispatch unchanged.
---
Nitpick comments:
In `@apps/ios/GuideDogs/Code/Visual` UI/View
Controllers/Tutorials/Markers/MarkerTutorialViewController.swift:
- Around line 138-141: Replace the duplicated background color literal in
MarkerTutorialViewController with a shared marker tutorial background constant,
such as UIColor.markerTutorialBackground. Update MarkerTutorialView to consume
the same shared palette value so both UIKit and SwiftUI views remain
synchronized when the color changes.
🪄 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: 23233c23-6821-481f-87b1-e4d2fdccc6e4
📒 Files selected for processing (9)
apps/ios/GuideDogs.xcodeproj/project.pbxprojapps/ios/GuideDogs/Code/Visual UI/View Controllers/Help/HelpViewController.swiftapps/ios/GuideDogs/Code/Visual UI/View Controllers/Home/HomeViewController.swiftapps/ios/GuideDogs/Code/Visual UI/View Controllers/Tutorials/Markers/MarkerTutorialViewController.swiftapps/ios/GuideDogs/Code/Visual UI/Views/Help.storyboardapps/ios/GuideDogs/Code/Visual UI/Views/MarkerTutorial.storyboardapps/ios/GuideDogs/Code/Visual UI/Views/Tutorials/MarkerTutorialView.swiftapps/ios/GuideDogs/Code/Visual UI/Views/main.storyboardapps/ios/UnitTests/MarkerTutorialViewStateTests.swift
💤 Files with no reviewable changes (4)
- apps/ios/GuideDogs/Code/Visual UI/Views/MarkerTutorial.storyboard
- apps/ios/GuideDogs/Code/Visual UI/Views/Help.storyboard
- apps/ios/GuideDogs/Code/Visual UI/View Controllers/Home/HomeViewController.swift
- apps/ios/GuideDogs/Code/Visual UI/Views/main.storyboard
Comment on lines
+94
to
+97
| defer { | ||
| controller.stop() | ||
| AppContext.shared.isInTutorialMode = false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the pre-test application and window state. These tests leave global state altered for later tests.
apps/ios/UnitTests/MarkerTutorialViewStateTests.swift#L94-L97: capture and restore the originalAppContext.shared.isInTutorialModevalue rather than always setting it tofalse.apps/ios/UnitTests/MarkerTutorialViewStateTests.swift#L145-L147: capture the scene’s previous key window beforemakeKeyAndVisible(), then make it key again during cleanup.
📍 Affects 1 file
apps/ios/UnitTests/MarkerTutorialViewStateTests.swift#L94-L97(this comment)apps/ios/UnitTests/MarkerTutorialViewStateTests.swift#L145-L147
🤖 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 `@apps/ios/UnitTests/MarkerTutorialViewStateTests.swift` around lines 94 - 97,
In apps/ios/UnitTests/MarkerTutorialViewStateTests.swift lines 94-97, capture
the original AppContext.shared.isInTutorialMode value before the test and
restore that captured value in the defer block instead of forcing false; at
lines 145-147, capture the scene’s previous key window before
makeKeyAndVisible() and restore it as key during cleanup.
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
MarkerTutorial.storyboardwith a scrollable SwiftUI tutorial surface while retainingMarkerTutorialViewControlleras the UIKit coordinator.Why
The marker tutorial UI was coupled to a dedicated storyboard and selector-based navigation contracts. Moving the presentation to SwiftUI removes that Interface Builder dependency while keeping UIKit responsible for the tutorial lifecycle, navigation, and side effects.
User impact
The marker tutorial keeps its existing appearance and flow, with improved dynamic layout and explicit accessibility semantics. Users continue to launch it from Help, select a POI, edit a marker, hear nearby markers, configure a beacon, and exit through the existing navigation stack.
Validation
InterfaceBuilderAudit: noMarkerTutorialstoryboard or scene references remain.git diff --checkpassed.