Repository navigation
ios: preview the walk's moves in onboarding - #152
Conversation
Adds a page between the walk page and the haze page that lists the five moves the walk asks for, in order: aim at the meter, walk along the wall, tilt down or up (with a second look from a step to the side), follow the blue ring and its edge arrow, and tap where the wall ends. Each step quotes the walk's cards from ScanCopy, and the picture draws the ring and the edge arrow with TargetMarker itself on the onboarding wall. The screen-state audit gains an onboarding-moves state (default and AX5) that pages to it and checks each move's words; the button-flow test taps Next once more. Fixes #85 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add house-scanning-master to your team's repositories. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e085372d90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Instruction( | ||
| title: "Walk along the wall", | ||
| detail: sentences(walk.detail, "If you're too close, the card says \"\(stepBack.title).\"") |
There was a problem hiding this comment.
When the wall walk begins near the meter and its ground is not already covered, GuidancePlanner.preferredTask returns .aimAtGround before considering either side's .walk task. This numbered preview instead puts “Walk along the wall” before “Tilt down or up,” contradicting the stated request order precisely on the normal first-run path; reorder these items or stop presenting them as a sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Confirmed at e085372: GuidancePlanner.preferredTask:239-241 asks for ground near the meter before the side walk at246 onward, while OnboardingScreen.moves lists walking before tilt. Please present these as possible moves without a numbered sequence, or put the initial ground request before walking and avoid implying a fixed order for later requests. Preserve the new page, Skip-to-safety and the matching UI expectations. This is a guidance-copy correction, not a blanket blocker for controlled testing; physical comprehension remains unverified.
There was a problem hiding this comment.
Fixed in b2d80e8. The moves page now lists them in the order GuidancePlanner.preferredTask asks for them at the start: 1. Aim at your meter, 2. Tilt down at your meter ("Tilt down to show the ground." Later the card may ask for this again, or say "Tilt up to show more wall." ...), 3. Walk along the wall, 4. Follow the blue ring, 5. Tap where the wall ends. The page body now says "After the first two, it asks for whatever the walk still needs", so no fixed order is implied for later requests. The new page and Skip are unchanged; the onboarding-moves UI expectations swap move 2 and move 3.
GuidancePlanner.preferredTask asks for the ground by the meter before either side's walk, so the moves page now lists the tilt second and stops promising a fixed order after the first two moves. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # ios/HouseScanUITests/ScreenStatesUITests.swift
2 participants
Fixes #85
What the old code did
Onboarding had three pages: the walk and its length, the haze (with a line about the ring from #81), and safety. Nothing before the camera named the moves the walk asks for: tilting down or up, a second look from a step to the side, following the ring and its edge arrow, and marking where the wall ends. In the 4.1 field test, testers met each move for the first time as a card they couldn't decode (#77, #81, #63).
What changed
OnboardingScreen.swiftgets a fourth page, "What the walk asks for", between the walk page and the haze page. It lists five numbered moves. The first two are in the order the walk asks for them (GuidancePlanner.preferredTaskasks for the ground by the meter before either side's walk); the page body says that after those, the card asks for whatever the walk still needs:ScanCopy.guidance(...)at build time instead of being retyped, so the page and the cards can't drift apart. "Wall ends here" is the button label inWallWalkScreen.WallDrawing. It adds the path dots asWayfindingOverlaydraws them, a marked wall end asWallMarksOverlaydraws one, the button's flag, and the ring and edge arrow. The ring and arrow areTargetMarkeritself, so any change to the camera's ring (The target ring and arrows show no progress, aren't explained, and the arrow can contradict the card #81, ios: aim ring fills as its stretch is captured, with a legend and an unambiguous arrow #117) shows up here too. The ring fills, turns green and starts again every 6 s. With Reduce Motion it stays half full.onboarding.move.1….5), and the number circle grows with Dynamic Type.pages.count - 1.ScreenStatesUITests.swift:onboarding-movesstate is audited at the default size and at AX5. It taps Next, waits for the page to slide into view, and checks that each move element carries the cards' words.testWholeFlowThroughTheButtonstaps Next three times instead of two.SourceRestartUITestsand the practice-meter tests use Skip, so they needed no change.This adds to #117 (now on
beta), which rewrote the haze page's ring line, without changing it.git merge-tree --write-tree origin/beta origin/t3/85-onboarding-movesmerges cleanly. The APIs this page uses (theScanCopy.guidancecases andTargetMarker(placement:progress:)) are the same onbeta. OnlyScreenStatesUITeststaps Next through onboarding; the other UI tests onbetause Skip.Known gaps
beta. Once The first "Tilt down… at your meter" task blocked the start in all three runs: no timeout, and the two-spots rule is never explained #77 and "Can't get there" reads wrong on tilt tasks; testers look for a "that's the ground" answer instead #63 change the tilt-card copy, check that move 2 still matches. The quoted titles followScanCopyautomatically, but the sentence around them doesn't.beta(ios: aim ring fills as its stretch is captured, with a legend and an unambiguous arrow #117, fix(ios): end a side at the phone; confirm and back out of the next wall (#71, #70) #136, ios: review follow-ups for #132 (AR double draw, result chevron, aim legend contrast) #137). The runs below are ate085372, before those merges.Tests
Round 1 (
b2d80e8, the move reorder from review):g2-tf1on the Mac mini. Drift,swift testand the device build are running; the UI results aren't in yet.No server or web files changed, so
make serverandmake webweren't run. iOS was checked on the Mac mini with~/hs-ci/ci-local.sh, which replicatesios.yml. Both runs are ate085372:g2-onboarding-r1ran the default UI suite, which skips the every-state audit. The xcodegen drift check, HouseScanKitswift testwith-warnings-as-errors, and the device build passed. Of 24 UI tests, 22 passed, 1 was skipped and 1 failed. The passes includetestWholeFlowThroughTheButtons, which now taps Next three times. The failure wasAuditIssueKeyTests.testAGoneElementIsReadWithoutRecordingAFailure: on the overloaded Mac mini, the tap on the ground question's "Gravel" answer didn't register, soground.changenever appeared. This branch doesn't touch that screen or that test.g2-onboarding-r1-fullran the full UI suite, including the every-state audit. Drift,swift testand the device build passed.testAGoneElementIsReadWithoutRecordingAFailurepassed this time, so the earlier failure was a flake. The every-state audit checkedonboarding-movesandonboarding-moves-AX5and reported no issues on either. Two tests failed, both outside this change:testEveryStatePassesTheAccessibilityAuditfailed onwallWalk-aim: the contrast ofaim.legend("…take a small step to one side…"). That is the legend at this branch's base, before ios: aim ring fills as its stretch is captured, with a legend and an unambiguous arrow #117. Onbeta, ios: aim ring fills as its stretch is captured, with a legend and an unambiguous arrow #117 reworded the legend and moved it onto the card's solid scrim.FullFlowUITests.testFullFlowFromLidarReplayfailed with "Audit failed to complete in time" on the wallWalk screen, on the overloaded Mac mini.testFullFlowFromReplaypassed in the same run.The remaining UI tests in this run were still going when this PR was updated.
UI tests on the branch merged with
beta, including the every-state audit underfull-ui, are pending on GitHub CI.Needs the
full-uilabel, because it changes a screen and its copy.🤖 Generated with Claude Code