[AidenJohnston]

Part of #81 (the overlay half; unit U4, #120, does the card copy).

Base: beta. Beta already carries #100 (U1) and #120 (U4), and the first commit of this PR (9704867, via "Merge non-LiDAR AR and aim guidance for beta testing"). Beta was merged back into this branch at fbab420, so what is left to land is the completed-ring hold and the review fixes.

What changed

The aim ring fills. During an aim task (aimAtGround, aimAtWall), the ring on the walk screen now fills clockwise as the task's stretch gets covered. Progress is the covered share of the task's band within GuidancePlanner.aimHalfWidth of its s, divided by GuidancePlanner.aimSatisfied. That is the same window and threshold the planner uses to call the task done. The value comes from the published CoverageStrip.coveredFraction, and the planner's two constants are read directly (no new literals). Every other step still draws the plain pulsing ring. The filling ring doesn't pulse, and VoiceOver reads it as "Spot to show, N percent captured" (aim.ring).

• ScanViewState+AimProgress.swift computes aimProgress, and aimProgress(for:) for any step. It lives in Runtime/ so that UI/ still doesn't import HouseScanKit. The brief put this in CameraOverlays. It doesn't touch ScanEngine or the planner.
• TargetMarker gets progress: Double? = nil. The call TargetMarker(placement:) on the AR result screen still compiles unchanged.

A completed ring holds green with a tick for 1 s. The engine publishes the coverage that completes an aim step and moves guidance to the next step in the same MainActor turn (ScanEngine.afterCoverageChange), so the screen never sees the finished step as current. CameraOverlays now scores the outgoing step against the new strip with aimProgress(for:). If it is complete, a full green ring with a tick stays at that spot for 1 s (completedHold) in place of the next step's marker, then the next marker shows. The frame that ends the step already draws the held ring, so the next step's marker doesn't flash in between. The ring keeps its identity, so it animates from its last fill to green. If the completed spot is off screen, nothing is held. No engine change.

The first aim ring comes with a legend. The first time a filling ring is on screen during a walk, a line appears above or below it: "Keep the ring in view and take one step to the side. It fills as your phone captures this spot." (aim.legend). It takes the side with more room when it appears and changes sides only once the other side has 40 pt more room, so pose jitter near the middle of the screen doesn't flip it. It keeps to the lane between the instruction card and the controls: where the whole line doesn't fit on its side (large text sizes), the short "It fills as your phone captures this spot." is drawn, and where neither fits, nothing. The legend goes away once that aim step ends and doesn't come back. The legend's state lives in CameraOverlays, so it resets when the screen changes. A gap request's ring has no progress, so the legend never shows there.

The off-screen marker is an arrow. It now uses arrow.right (an arrow with a shaft) instead of chevron.right. The tester's "check-mark bubble" is most likely the chevron turned to point down in a blue disc. That is inferred from the report, not confirmed. The battery-spot marker on "See it on your wall" (#100) picks up the same arrow. The arrows and the legend share one lane (laneTop, laneBottomInset).

Onboarding explains the ring and the arrows. The second page ("Your phone takes the photos") adds a line with the ring's icon: "A blue ring shows where to go or look. When it asks you to show a spot, it fills as your phone captures it and turns green when done. If it's off screen, an arrow at the edge points to it." (onboarding.ring). It covers both kinds of ring: the walk-goal, mark-end, tilt-up and see-behind rings only mark a place and don't fill.

Why

In the 4.1 field test the tester asked several times what the blue circle wanted, and held it on target with no response. The ring never changed, even though the planner knew how much of its stretch was still missing. The tester also asked for the circle to go green once captured.

The legend's wording differs from the brief. Please decide which to keep. The brief's line was "Put the dot in the ring and hold still. It fills as the view is captured." Holding still can't fill the ring. A cell counts as covered only once it is seen from two positions at least CoverageConfig.coveringBaseline (0.25 m) apart, so a phone held still adds nothing. The walk screen also has no dot of its own outside marking, so "the dot" would mean the ring's own centre. The line in this PR asks for one step to the side instead: a shuffle of a few inches keeps no new view (views are kept after about 0.5 m of moving), so "a small step" could leave the ring unchanged. It lives in ScanCopy.aimRingLegend, so changing it is a one-line edit.

Wording to agree with #120 (now in beta). The tilt-down card says "Take a small step sideways as you look.", and the hint title says "Now take one step to the side and look again". The legend now uses the hint title's words. On the first tilt-down the card and the legend both ask for a step, in slightly different words. The card copy belongs to U4, so this PR leaves it alone; one owner for the step instruction is still a team call.

UI changes

No captures: there is no Simulator on this machine. The new demo states render the changed screens:

• -uiDemo -uiDemoFreeze -uiDemoPhase wallWalk -uiDemoAim: the ring is half full (2 of 5 ground cells covered, where 80 % completes it), with the legend.
• -uiDemo -uiDemoFreeze -uiDemoPhase wallWalk -uiDemoAimOffScreen: the same task for ground left of the view, so the edge arrow shows.
• Without -uiDemoFreeze, -uiDemoAim fills the ring one cell per tick. The tick that fills it also moves the walk on, as the engine does, so the demo shows the same 1 s green hold a phone shows.

The UI test run attaches screenshots of both states to its result bundle (wallWalk-aim, wallWalk-aimOffScreen).

Verification

Verified on the Mac mini (ci-local.sh t3/41-target-ring target-ring at b2c6367, after merging beta at 5bc81fb): PASS.

• The xcodegen drift check passed (no project changes).
• swift test for HouseScanKit passed.
• The device build passed, so the branch compiles under Swift 6 with strict concurrency.
• HouseScanUITests ran on an iPhone 17 Simulator (iOS 26.5): 23 tests, 0 failures, 1 skipped.
  • wallWalk-aim exposes aim.ring with the value "50 percent captured".
  • testAimRingShowsProgressAndItsLegend finds the legend on the on-screen state and neither element on the off-screen one.
  • testWholeFlowThroughTheButtons passes with the demo's same-tick aim completion.

Also checked by reading the code:

• Every changed call site: WayfindingOverlay (built only in CameraOverlays, with the new completed: and legendShort:), TargetMarker, CameraOverlays and aimProgress.
• The demo numbers: at s = 0.4, 2 of 5 ground cells are covered, which gives 50 %. The tolerance narrowing (cellWidth * 1e-3) doesn't move any cell edge across the window there.

Not yet verified:

• testEveryStatePassesTheAccessibilityAudit, the one skipped test. ci-local.sh skips it unless FULL is set. It includes wallWalk-aim at AX5, where the legend now drops to the short line or to nothing, so it needs a GitHub iOS run or a FULL=1 local run.
• The 1 s hold has no UI test: a one-second window is too short to poll reliably.
• How it feels on a phone:
  • whether the fill rate reads as progress while stepping sideways;
  • whether the legend sits clear of the instruction card and the controls on small screens;
  • whether 1 s of green reads as done without holding up the next step.

Risk

• aimProgress narrows its window by the coverage map's overlap tolerance (CoverageMap.indices(overlapping:)), so a covered cell that only touches the window's edge no longer counts on the ring while the planner ignores it.
• The strip covers only the wall map's visible range (CoverageMap.visibleRange), and the planner checks the whole map. Near the edge of that range the two counts can differ by a cell. The ring can then read slightly above or below the planner's decision, and a completed step can miss its green hold.
• For 1 s after an aim step completes, the next step's ring or arrow is replaced by the green ring (the path still shows).
• If a later change adds associated values to GuidanceStep.aimAtGround or aimAtWall, the switch in ScanViewState+AimProgress.swift needs the matching patterns.
• The arrow change also affects the battery-spot marker on the AR result screen (ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100). The marker's position and API are unchanged.

Files touched

• ios/HouseScan/UI/Camera/WayfindingOverlay.swift: TargetMarker progress ring and arrow; WayfindingOverlay progress, the held completed ring, the legend with its side kept, onLegendShown, and the shared lane insets.
• ios/HouseScan/UI/Camera/CameraOverlays.swift: passes the progress through, runs the legend's show-once state, and holds a completed ring for 1 s.
• ios/HouseScan/Runtime/ScanViewState+AimProgress.swift (new): aimProgress and aimProgress(for:).
• ios/HouseScan/UI/Copy/ScanCopy.swift: aimRingLegend and aimRingLegendShort only, in their own section after coaching. The aim-task card copy is untouched.
• ios/HouseScan/UI/Screens/OnboardingScreen.swift: the ring and arrow line.
• ios/HouseScan/UI/Preview/DemoEngine.swift, UIDemo.swift: -uiDemoAim and -uiDemoAimOffScreen; the aim step ends in the tick that fills it.
• ios/HouseScanUITests/ScreenStatesUITests.swift: the two states, the ring value check and the legend test.

No changes to project.yml or project.pbxproj. The new file sits in the synced HouseScan/ folder.

Dependencies

• ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100 (U1) and ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 (U4, the other half of The target ring and arrows show no progress, aren't explained, and the arrow can contradict the card #81) are both in beta already. The one merge conflict with beta (ScanCopy.swift, both sides adding after guidance(_:)) is resolved in fbab420: beta's helpers stay, the legend sits in its own section after them. The step wording still overlaps (see Why).

Checklist

• The scope is focused; related changes belong together
• I explained what changed and why
• I included before/after images for visible changes. N/A for now: no Simulator here. The UI test attaches wallWalk-aim and wallWalk-aimOffScreen screenshots to its result bundle, and before-and-after captures from CI or a device should be added before review.
• I included a video for motion or interaction changes. N/A for now for the same reason. The fill and the green hold need a device or Simulator recording of -uiDemoAim without -uiDemoFreeze.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[AidenJohnston]

Adversarial review of U7 (#81 overlay half), head 9704867, base t3/41-ar-canvas-fallback (#100 head 41291e8, which is an ancestor, so the stack is current).

Must fix

1. On a phone, the green ring and tick never show, but onboarding tells the homeowner to expect them. Refs: ios/HouseScan/UI/Camera/WayfindingOverlay.swift:200-201, ios/HouseScan/UI/Screens/OnboardingScreen.swift:25, ios/HouseScan/Runtime/ScanEngine.swift:616-623.

   • Why: every kept frame goes through afterCoverageChange, which calls publishCoverage() and then updateGuidance() in the same synchronous MainActor call. When the new coverage meets the aim task, GuidancePlanner.update switches task at once (satisfied skips minDwell). So state.coverage with the stretch at ≥ 80 % and state.guidance == .aimAtGround/.aimAtWall for that stretch never reach SwiftUI in the same render.
   • Result: aimProgress >= 1 is never drawn live. The done branch (Palette.covered arc plus the checkmark) only runs in the demo, where walkScript holds it for one 0.45 s tick. On a phone the ring climbs to at most the last value under 1 (for example 75 %) and then disappears or jumps to the next target.
   • Scenario: the homeowner reads "It fills and turns green once captured" on onboarding page 2, then watches the ring stop at 3/4 and vanish without turning green. That is the tester's own complaint in run 1 at 0:47 ("the circle goes green once it's captured"), and they will again wait for a signal that never comes.
   • The onboarding line also says "A blue ring marks a spot to show. It fills…" about every ring. The walk, mark-end, tilt-up and see-behind rings are still the plain pulsing ring and never fill.
   • The PR body says "on a device the tick may only flash". It won't render at all.
   • Fix, either:
     • (a) Latch the completion in UI only, without touching ScanEngine or the planner: in CameraOverlays, onChange(of: state.guidance) from an aim step whose stretch now reads ≥ 1 against the new state.coverage keeps drawing the done ring at the old target for about 0.8 s.
     • (b) Drop "and turns green once captured" from the onboarding line, scope it to the tilt steps ("During a tilt step, the ring fills as your phone captures the spot…"), and correct the PR body.

   With (b), say in the PR that the green/tick branch is reachable only in the demo, or remove it.

Should fix

2. "take a small step" may never fill the ring. Ref: ios/HouseScan/UI/Copy/ScanCopy.swift:71.

   • A cell counts as covered only after two sightings at least CoverageConfig.coveringBaseline (0.25 m) apart.
   • A second sighting needs a kept frame, and AutoCaptureConfig keeps one only after spacingMeters 0.5 m or spacingRadians 15°, or for 3 newly seen cells. None of those apply while re-aiming at an already-seen stretch.
   • Scenario: a homeowner 1.5 m from the wall steps about 20 cm sideways, keeps the ring centred and turns the phone well under 15°. No frame is kept, so the ring stays put, as in run 2 (0:28–1:29), where consecutive kept views were mostly under 25 cm apart.
   • Suggest "take one step to the side" (the run-2 analysis wording), pending the team's copy decision, which the PR already flags.
   • Check it against U4 and #120's card copy so the legend and the card don't give different instructions for the same step.

3. The legend's height is unbounded at large text sizes, and that size isn't tested. Refs: ios/HouseScan/UI/Camera/WayfindingOverlay.swift:49-54, ios/HouseScanUITests/ScreenStatesUITests.swift:63.

   • Placement assumes the card ends at y = 260 and the controls start 300 pt from the bottom. The text is .subheadline, fixedSize(vertical:), 300 pt wide, and inside a 400 pt frame.
   • At AX sizes that is 8–10 lines, which runs over the instruction card or the tape/controls. The legend is drawn under CameraChrome, so the chrome paints over it and the legend is cut off.
   • Fix: add wallWalk-aim to largestTextStates and cap the legend, for example .dynamicTypeSize(...DynamicTypeSize.accessibility2) or lineLimit + minimumScaleFactor, or clamp its reach to the room computed from roomAbove/roomBelow.

Notes

4. The ring's count and the planner's decision can disagree. Refs: ios/HouseScan/Runtime/ScanViewState+AimProgress.swift:26 vs ios/HouseScan/Contract/ScanContract.swift:266.
   • CoverageStrip.coveredFraction counts any cell whose closed range touches the window, with no tolerance and no ends filter. The planner uses CoverageMap.indices(overlapping:), which needs more than 1/1000 of a cell of overlap, and filters through allows.
   • Scenario: the planner has 3 of 4 cells covered (not met) while the strip also counts a covered neighbour that touches the window edge to within float error, 4 of 5. The ring then shows the green tick while the task stays up.
   • This is rare (it needs s ± 0.3 to land on a cell edge), and ironically it is the only way the tick shows on a phone. Mirroring the map's tolerance in aimProgress would close it; the PR's Risk section covers only the visibleRange part.
5. The legend's show-once state lives in CameraOverlays @State (CameraOverlays.swift:18-19), so it comes back whenever WallWalkScreen is rebuilt. The brief said once per scan. This is acceptable for now because aim steps occur only in wallWalk, and the PR states it.
6. Doc comments in ResultARScreen.swift (lines 8, 82, 91, 124) still say "chevron" now that the glyph is arrow.right. That file belongs to U1/#100, so fix it there or after merge, not here.
7. Mental compile found no problems:
   • The TargetMarker(placement:) call in ResultARScreen still matches the new memberwise init.
   • WayfindingOverlay is built only in CameraOverlays.
   • The OnboardingPage argument order matches.
   • GuidancePlanner.aimHalfWidth and aimSatisfied are public static.
   • The switch in aimProgress has a default, and the extension inherits @MainActor from ScanViewState.
   • The if let …, case … = in the ViewBuilder, and trim/inset/accessibilityValue, match the existing MeterCloseUpScreen ring.
   • The demo numbers check out: firstCellS −3.2, cells 21–25 overlap 0.1…0.7, cells 21–22 are covered, and 0.4/0.8 is exactly 0.5, giving "50 percent captured".
   • The UI test states run with -uiDemoFreeze, so the value check is deterministic.
   • SpotDirection wraps the marker in accessibilityElement(children: .ignore), so hiding the arrow from accessibility changes nothing there.
8. Scope and public-repo checks pass:
   • No project.yml or project.pbxproj edits.
   • ScanEngine, GuidancePlanner, ResultARScreen and the aim card copy are untouched.
   • The new file sits in the synced HouseScan/ folder.
   • The diff, commit and PR body contain no names, meter numbers, addresses or private/ content.
   • The PR body has "Part of #81", the base, a verified vs CI/device split, files touched, dependencies and the attribution line.
   • The legend wording change from the brief is surfaced as a team decision, not decided silently.
9. #120 (walk-task hold) keeps the same aimHalfWidth/aimSatisfied window, so aimProgress stays consistent with it. Re-check the switch if U4 adds associated values to the aim steps.
10. Checks: only "Label PR size" and CodeRabbit (skipped for draft) have run. iOS build is skipping because iOS drafts get no PR-triggered run. Nothing here has been compiled. Run the iOS workflow manually, including ScreenStatesUITests with the two new states, before this leaves draft.

Verdict: blocked

🤖 Generated with Claude Code

[AidenJohnston]

Adversarial review of #117 (U7, Part of #81) at 9704867, base t3/41-ar-canvas-fallback (#100 head 41291e8, in sync). I read the diff, the full changed files, the engine and planner around them, #81 and its 4.1 run analyses, and #120 (U4).

Must fix

1. The ring never reaches green with a tick on a device. Onboarding promises that it does.
   • Where: ios/HouseScan/Runtime/ScanViewState+AimProgress.swift:27, ios/HouseScan/UI/Camera/WayfindingOverlay.swift:200-221, ios/HouseScan/UI/Screens/OnboardingScreen.swift:25.
   • Why: ScanEngine.afterCoverageChange (ScanEngine.swift:616-622) calls publishCoverage() and then updateGuidance() synchronously in the same MainActor turn. GuidancePlanner.update switches away from the current task as soon as isSatisfied holds, with no dwell. That uses the same >= aimSatisfied test this PR divides by. So state.coverage crossing 80 % and state.guidance leaving the aim step land in a single SwiftUI update, and aimProgress is then computed for the next step (or is nil). progressRing with done == true is never drawn for the step that just finished.
   • Failure scenario: a stretch has 4-5 cells within +/-0.3 m, so the ring steps 0 -> 25 -> 50 -> 75 % (or up to 94 % with 4 cells). Then it vanishes or jumps to the next target. The tester's own ask in run 1 (0:47) was "the circle goes green once it's captured", and that still never happens. Onboarding now says "It fills and turns green once captured", which the user will never see.
   • The demo hides this. DemoEngine.walkScript (DemoEngine.swift:434) waits a full 0.45 s tick at progress 1 before refreshGuidance(), unlike the real engine.
   • The PR body says the tick "may only flash". It won't render at all.
   • Expected fix, UI side only, no engine change: in CameraOverlays, on onChange(of: state.guidance) away from .aimAtGround/.aimAtWall, recompute the outgoing step's progress against the new state.coverage. It is the same formula, so factor out aimProgress(for: GuidanceStep). If it is >= 1, keep a completed TargetMarker(placement:progress: 1) at the last on-screen point for about 1 s. Make the demo switch in the same tick as the real engine so the demo shows what a device shows. If the team doesn't want the hold, drop "turns green" from the onboarding line and from the PR body instead.

Should fix

1. The legend flips between above and below the ring near screen centre.

   • Where: WayfindingOverlay.swift:49-51.
   • Why: below = roomBelow >= roomAbove is recomputed on every frame. The two sides are equal at y = (height - 40) / 2, which is 20 pt above screen centre (406 pt on an 852 pt screen). That is where a homeowner naturally holds the ring. ARKit pose jitter of a few points moves the ring across that line, and the legend jumps across the ring each time.
   • Fix: pick the side once when the legend appears (keep it in CameraOverlays next to legendStep), or add hysteresis, e.g. switch only when the other side has 40 pt or more extra room.

2. The onboarding line describes every blue ring, but most rings don't fill.

   • Where: OnboardingScreen.swift:25.
   • Why: TargetMarker gets progress == nil, so the plain pulsing ring that never fills, for .walk (the walk goal at reach + 0.9 m), .markEnd, .tiltUp and .seeBehind.
   • Failure scenario: a user who read "A blue ring marks a spot to show. It fills and turns green" walks right, and the walk-goal ring sits there unchanged. That is the same confusion #81 reports ("What does this blue circle want from me?").
   • Fix: word it so it covers both kinds, e.g. "A blue ring shows where to look. When it asks you to show a spot, it fills as your phone captures it", or make the ring look different when it can fill.

3. Conflicts and duplicated copy with #120 (U4, the other half of #81).

   • Where: ios/HouseScan/UI/Copy/ScanCopy.swift:67-71.
   • Why: #120 inserts its aim(_:ground:hint:) helper at the same place, after guidance(_:) and before // MARK: Coaching, so these two PRs will conflict on the text.
   • #120 also changes the aimAtGround card detail to "... Take a small step sideways as you look." and adds "Now take one step to the side and look again". With both merged, the first tilt-down shows the step instruction twice, once on the card and once in the legend. Next to #120's "Put the ring on the strip", "Keep the ring in view" is a third phrasing.
   • Fix: agree the wording with the U4 owner. Name #120 in the Dependencies section (it currently says "U4 ... touches different functions", but they share a file and an insertion point).

4. The legend's layout constants duplicate the arrow lane's.

   • Where: WayfindingOverlay.swift:49-50 against :121.
   • Why: 260 and 300 are copied from the lane in placement(for:in:), and the doc comment says the legend keeps to "the band the off-screen arrows keep to". If one changes, the legend and arrows no longer share a band, and the legend can slide under the instruction card.
   • Fix: hoist the lane insets into shared static lets.

Notes

• The Swift mental compile looks clean:
  • GuidancePlanner.aimHalfWidth (Float) and aimSatisfied (Double) are public static let.
  • CoverageStrip.coveredFraction(_:in:) and CoverageBand are app-side types with no HouseScanKit name clash.
  • The extension inherits @MainActor from ScanViewState, and GuidanceStep is Equatable for onChange.
  • .transition(reduceMotion ? .opacity : .scale(...).combined(with: .opacity)) matches the existing GapRequestScreen usage, and if let legend, case .onScreen(...) = marker is valid in a ViewBuilder.
  • OnboardingPage's memberwise init still accepts the three existing calls.
  • Every TargetMarker( / WayfindingOverlay( / CameraOverlays( call site still matches (ResultARScreen.swift:102, CameraOverlays.swift:40, WallWalkScreen.swift:22, GapRequestScreen.swift:20).
  • Gap screens publish guidance = .gap, so no progress and no legend there. Confirmed.
• I recomputed the demo numbers with float32 cell edges. At s = 0.4, cells 21-25 overlap the window and 2 are covered, so 0.4 / 0.8 = "50 percent captured". At s = -2.4, 0 are covered. I projected the target by hand to about (175, 433) on a 393 x 852 screen, inside the ring bounds, with radius 30. So the new expectation and testAimRingShowsProgressAndItsLegend should hold. testWholeFlowThroughTheButtons never enters an aim step in the demo, so it is unaffected.
• Moving .accessibilityHidden(true) from the overlay root down to its parts means any future subview of WayfindingOverlay is exposed to VoiceOver by default. That is fine, but worth a comment on the struct.
• Scope: nothing touches project.yml or project.pbxproj, ScanEngine, GuidancePlanner or ResultARScreen. Only aimRingLegend is added to ScanCopy. The new Runtime/ file (instead of the brief's CameraOverlays) is explained in the body, and I agree with it because it keeps UI/ off HouseScanKit. The legend wording differs from the brief ("hold still" can't fill a cell because of coveringBaseline). It is flagged openly for a decision, not made silently, and I agree with the reasoning.
• The "check-mark bubble = rotated chevron" cause is correctly labelled as inferred. Note that this PR now adds a real checkmark to the walk screen (the completed ring). If Must fix 1 makes it visible, it is explained by onboarding, which is the right order.
• Public-repo check on the diff, PR body and commit message: no names, meter numbers or serials, addresses, emails, private/ content or media. The fixtures are the synthetic DemoScene.
• PR body: has Part of #81, the base and merge order, verified vs CI/device-only, files touched, dependencies and the attribution line. The screenshot and video checklist items are open (no Simulator).
• Checks: Label PR size passed. CodeRabbit skipped (draft). iOS build is skipping, because drafts get no PR-triggered iOS run. Nothing in this PR has been compiled or run, so the Swift 6 build and ScreenStatesUITests (the audit on wallWalk-aim / wallWalk-aimOffScreen) must go green before this leaves draft.

Verdict: blocked (Must fix 1: the completion state the issue asks for, and onboarding now promises, can't appear on a device)

🤖 Generated with Claude Code

[AidenJohnston]

Addressed the review at 9704867 in 6d82e62.

Must fix

1. The ring never turns green with a tick on a device. Fixed on the UI side, with no engine change.
   • ScanViewState+AimProgress.swift: factored out aimProgress(for: GuidanceStep). aimProgress now calls it with guidance.
   • CameraOverlays: watches the step and its target together (AimSnapshot, so the change still has the outgoing target). When the step changes, it scores the outgoing aim step against the new state.coverage. If that is >= 1, it holds the old target as completedTarget for 1 s (completedHold, cleared by a .task(id:) that a newer completion cancels).
   • The frame that ends the step draws before onChange runs, so heldTarget also derives the hold from lastAim in body. Without that, the next step's marker would flash for a frame between the filling ring and the green one.
   • WayfindingOverlay gets completed:. While that target is on screen it draws TargetMarker(placement:progress: 1) in place of the live target. It uses the same TargetMarker in the view tree, so the ring animates from its last fill to green and the tick. If the completed spot is off screen, nothing is held. The legend is suppressed during the hold.
   • Demo: walkScript now covers the cell and calls refreshGuidance() in the same tick once the ring is full, as the engine does. It also no longer calls refreshCoverage() there, because that rebuilt the strip and could drop the cells that completed the ring. The demo now shows the same hold a phone shows.
   • Onboarding and the PR body keep "turns green". The body's "may only flash" line is gone and the hold is described.

Should fix

1. The legend flips sides near screen centre. Fixed with hysteresis. WayfindingOverlay keeps @State legendBelow, set when the legend appears. It changes sides only when the other side has legendSwitchMargin (40 pt) more room, and resets when the legend disappears.
2. The onboarding line describes every blue ring. Reworded to cover both kinds: "A blue ring shows where to go or look. When it asks you to show a spot, it fills as your phone captures it and turns green when done. If it's off screen, an arrow at the edge points to it."
3. Conflict and duplicated copy with ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120. Partly addressed.
   • Moved aimRingLegend out of the guidance section into its own // MARK: Aim ring section after coaching, away from ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120's insertion point.
   • ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 is named in the Dependencies section, and the Why section flags the duplicated step instruction.
   • I did not change the wording. Choosing between the card, the hint titles and the legend is a call for the U4 owner and the team, and without ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 the legend is the only place that asks for the step.
4. The legend's layout constants duplicate the arrow lane's. Fixed. laneTop (260) and laneBottomInset (300) are shared static lets used by both placement(for:in:) and legendView.

Notes

• Added the suggested comment on WayfindingOverlay about each part setting its own accessibility.
• Still not compiled here (no Swift toolchain). I re-read every call site of what changed:
  • WayfindingOverlay(: only CameraOverlays. The new completed: argument sits between progress: and legend:, in declaration order.
  • TargetMarker(: WayfindingOverlay and ResultARScreen:102. Both are unchanged in shape.
  • CameraOverlays(: WallWalkScreen and GapRequestScreen. The new private @State properties have defaults, like FogOverlay's.
  • aimProgress: CameraOverlays and DemoEngine.
• The frozen demo states and testAimRingShowsProgressAndItsLegend are unaffected: the frozen state never ticks. The 1 s hold has no UI test, because a one-second window is too short to poll reliably. It needs a device or an unfrozen Simulator run of -uiDemoAim.
• The iOS build and ScreenStatesUITests still have to go green before this leaves draft.

🤖 Generated with Claude Code

[codemetrics-ai]

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.

[SamGu-NRX]

@AidenJohnston Concrete combined-beta failure for the existing #117 owner: run36302099002 at5bc81fb failed ScreenStatesUITests.swift:503 in the every-state audit, wallWalk-aim, Contrast on aim.legend: "Keep the ring in view and take a small step to one side. It fills as your phone captures this spot." FullFlow passed. The lead is extracting the exact screenshots.

I inspected latest head6d82e62a9c1d08a2247694d36e0b3be7ae05673c. WayfindingOverlay.swift:84-101 still uses white text, black at0.7 opacity and an opacity transition. Its newer placement hysteresis/completion changes do not directly repair contrast. This identifies the remaining path, not the root cause; a steady 70% black backing and a partially faded or overlapped frame need different fixes. Please inspect the failing frame and repair the legend's backing/transition or overlap as demonstrated, preserving readable VoiceOver text and the ring. Send the corrective SHA to the lead for the existing serial candidate run. No new CI or blind retry requested.

[SamGu-NRX]

@AidenJohnston I inspected the exact5bc screenshots from run36302099002. Please keep two findings separate:

1. The recorded audit failure is normal-size wallWalk-aim, not AX5. Frame83C656E5-60EA-46CB-9137-66AD4EACF195.png shows white legend text on dark backing with no obvious visual cause. The image does not establish fading as the cause.
2. AX5 frameDC660400-AA23-4F56-A85A-D39F2DBEB621.png visibly shows the enlarged legend extending above the screen and behind the top instruction/replay chips. This is a confirmed independent layout defect. At latest6d82e62, the fixed260/300pt lane and400pt legend frame do not account for the enlarged instruction card. Please make the legend fit the actual available area at accessibility sizes or place it in the scrollable instruction layout while preserving its meaning and VoiceOver. Do not hide the audit finding.

Both frames are in the lead's exported candidate-5bc81fb-screens evidence and the hosted result bundle. Root owns the next full run36304842990; no duplicate CI requested.

[AidenJohnston]

Fixer pass on both reviews at 9704867, on top of the earlier fix 6d82e62. Head is now b2c6367.

Merge with beta

• fbab420 merges origin/beta (5bc81fb) into this branch. Beta already had ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100, ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 and this PR's first commit, so the PR now shows only the completed-ring hold and the review fixes.
• The one conflict was ScanCopy.swift, where both sides added code after guidance(_:). Beta's aim(_:ground:hint:) and coaching helpers (coachingNote, coachingReplacesTask, withCoaching) are kept as they are. aimRingLegend sits in its own // MARK: Aim ring section after them.
• DemoEngine, UIDemo and OnboardingScreen merged cleanly. Nothing touches project.yml or project.pbxproj.

Review 1 (05:36)

1. Must fix, the green ring and tick never show on a phone. Fixed in 6d82e62 with option (a), unchanged here. CameraOverlays scores the outgoing aim step against the new strip and holds the completed ring for 1 s. DemoEngine now moves on in the same tick that fills the ring, as the engine does. The onboarding line covers both kinds of ring.
2. Should fix, "take a small step" may never fill the ring. Fixed in b2c6367. The legend now reads "Keep the ring in view and take one step to the side. …", using the words of ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120's hint title. Not changed: the tilt-down card's own "Take a small step sideways as you look." That is U4's card copy, so it's flagged in the PR body for the team.
3. Should fix, the legend's height is unbounded at large text sizes. Fixed in b2c6367.
   • The legend's frame is now the room between the ring and the lane's edge on its side, instead of a fixed 400 pt.
   • A ViewThatFits draws the full line if it fits, else ScanCopy.aimRingLegendShort ("It fills as your phone captures this spot."), else nothing. It can no longer run under the instruction card or the controls.
   • I didn't use a dynamicTypeSize cap, because the audit can report capped text as partially unsupported Dynamic Type.
   • wallWalk-aim was already in largestTextStates on beta, so the AX5 audit covers this state.
4. Note, the ring's count and the planner can disagree at a cell edge. Fixed in b2c6367. aimProgress narrows its window by the map's tolerance (cellWidth * 1e-3, as in CoverageMap.indices(overlapping:)). The ends filter isn't mirrored: the strip's cells outside the ends are never covered, so they can only lower the ring's count, never show it done early.
5. Note, the legend's show-once state lives in @State. Not changed. It is acceptable per the review and stated in the body.
6. Note, "chevron" in the ResultARScreen doc comments. Not changed here. That file belongs to U1/ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100, as the review says.
7. to 9. Notes, mental compile, scope and ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 consistency. No action needed. aimProgress's switch still matches: ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120 added no associated values.
8. Note, never compiled. Now built and tested on the Mac mini; see below.

Review 2 (06:57)

1. Must fix, completion never visible. Same as review 1, item 1. Fixed in 6d82e62.
2. Should fix 1, the legend flips sides. Fixed in 6d82e62 with hysteresis (legendSwitchMargin, 40 pt). The lane clamp in b2c6367 keeps the chosen side.
3. Should fix 2, the onboarding line covers every ring. Fixed in 6d82e62.
4. Should fix 3, the conflict and duplicated copy with ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120. The conflict is resolved in fbab420. The legend now shares ios: hold a walk aim until it is met or stalls, and ask for the meter ground only near the meter #120's "one step to the side" wording. The card-versus-legend duplication on the first tilt-down is left for the U4 owner and the team.
5. Should fix 4, duplicated lane constants. Fixed in 6d82e62 (laneTop, laneBottomInset). The legend's clamp in b2c6367 uses the same constants.
6. Notes. The accessibility comment on WayfindingOverlay was added in 6d82e62. The other notes need no change.

Verification

Mac mini, ci-local.sh t3/41-target-ring target-ring at b2c6367: PASS.

• The xcodegen drift check, swift test for HouseScanKit and the device build all passed. The build compiles Swift 6 with strict concurrency.
• HouseScanUITests on an iPhone 17 Simulator (iOS 26.5): 23 tests, 0 failures, 1 skipped. That run includes testAimRingShowsProgressAndItsLegend, which still finds the legend at default size, and testWholeFlowThroughTheButtons.
• Not covered: the skipped test is testEveryStatePassesTheAccessibilityAudit. ci-local.sh skips it unless FULL is set, so the wallWalk-aim AX5 audit with the new lane-clamped legend still needs a GitHub iOS run or FULL=1.
• Still device-only: how the 1 s green hold and the legend feel on a phone.

🤖 Generated with Claude Code

[SamGu-NRX]

@AidenJohnston One bounded source follow-up at b2c636738f63b95ebdb77b9fa5043d95ad5a0fa2, after inspecting the normal and AX5 frames from the combined beta run.

WayfindingOverlay.swift:99-102 calls onLegendShown on the enclosing ViewThatFits, including when neither text fits and it chooses Color.clear. CameraOverlays.swift:75-77 then records legendStep, and lines30-32 retire the legend when that step changes. An invisible first legend can therefore consume the one-time explanation. Please put the callback on the displayed text candidates, retaining side bookkeeping on the container, and check an initially hidden legend followed by a later aim with room.

The finite frame addresses overflow outside the numeric lane, but its260/300pt bounds still do not measure the enlarged instruction card. Normal-size contrast remains unproven because the white text and70% black backing are unchanged. Please retain the full audit and check both observed cases; the reported local run skipped that audit. The new legend also asks for one step while the tilt-down card still asks for a small step.

The current combined run36304842990 is already running at4896e0a and will finish unchanged. These follow-ups stay in the existing117 branch; no duplicate writer or CI run is requested.

[AidenJohnston]

Adversarial re-review of #117 at b2c6367 (base beta). I read the diff against origin/beta, both earlier reviews, the fixer's replies, Sam's two comments, the failed iOS run on this branch (36303117885, at 6d82e62), and t3/41-review-followups-2.

Must fix

1. The accessibility audit still fails on this branch, and this head doesn't touch the failing element.

   • Where: ios/HouseScan/UI/Camera/WayfindingOverlay.swift:116 (.foregroundStyle(.white)) and :120 (Color.black.opacity(0.7)).
   • Evidence: this branch's own workflow_dispatch run 36303117885 at 6d82e62 failed ScreenStatesUITests.swift:342 with wallWalk-aim: Contrast failed (id 'aim.legend' ...).
     • That is the same failure as beta's run 36302099002, at the default text size. The AX5 variant did not fail.
     • b2c6367 only moves these two lines into legendText. At the default size the demo still draws the full line, so the finding is unchanged.
   • Failure scenario: this is marked ready because the PR body says "Mac mini PASS". That run skipped testEveryStatePassesTheAccessibilityAudit (no FULL), so the first full run fails on wallWalk-aim again.
   • Overlap: t3/41-review-followups-2 (5c83ad4) changes exactly these two lines, to Palette.chalk on ScrimShape.rounded(14).
     • git merge-tree of that branch against this head is clean, and its change lands inside the new legendText.
     • Pick one owner, and don't write a second, different contrast fix here. Either land followups-2 first and then merge beta here, or cherry-pick 5c83ad4's hunk.
     • Then run the full audit (FULL=1 or GitHub) on the result. Sam notes the root cause isn't established ("no obvious visual cause" in the frame), so the scrim swap needs a green audit, not an assumption.

2. At accessibility text sizes the legend still sits behind the enlarged card, or disappears. This is the AX defect Sam confirmed, and the new code only clamps it to a fixed lane.

   • Where: WayfindingOverlay.swift:57 and :81 (fixed laneTop = 260), and :93-98 (reach and the Color.clear fallback).
   • What was asked: Sam's 08:04 comment said "the fixed 260/300pt lane and 400pt legend frame do not account for the enlarged instruction card". It asked for the legend to fit the actual available area "while preserving its meaning and VoiceOver. Do not hide the audit finding."
   • What happened: the 08:12 fixer reply doesn't respond to that comment. b2c6367 keeps the fixed 260 and adds a fallback that draws nothing.
   • Failure scenario A (AX1 to AX3, on a phone):
     • The tilt-down ring sits below centre, for example at y ≈ 520 on an 852 pt screen. Then roomAbove ≈ 230 and reach ≈ 218.
     • The full or short line fits that 260-based room, so it is drawn up to about y 280-300.
     • At these sizes the CameraChrome card extends well past 260 and paints over this layer, so the legend is partly covered.
     • That is the DC660400 frame again, at sizes the audit never runs (only AX5 is audited).
   • Failure scenario B (AX4 and AX5): nothing fits, so aim.legend is removed. The AX5 audit then passes because the element is gone, not because it fits. The people who enlarged their text get no explanation of the ring, which is the hiding the lead asked to avoid.
   • Fix, two parts:
     • Take the lane top from the card's measured bottom, for example with a preference key out of CameraChrome, or with the height passed through CameraOverlays.
     • When no wording fits beside the ring, keep the meaning somewhere readable. Options: the scrollable instruction stack at AX sizes, or at minimum aim.ring's accessibilityHint, so VoiceOver still conveys it.

Should fix

1. The legend counts as shown even when nothing is drawn, so it can retire unseen.

   • Where: WayfindingOverlay.swift:99-102. onAppear, and with it onLegendShown, sits on the ViewThatFits container, so it fires when the Color.clear branch wins.
   • Effect: CameraOverlays.swift:76 sets legendStep, and CameraOverlays.swift:32 retires the legend once that step ends.
   • Failure scenario: on a 667 pt screen (iPhone SE class, and #100 now supports non-LiDAR phones), the lane is max(667-560, 80) = 107 pt.
     • With a 30 pt ring radius and the 12 pt gap, reach is at most about 35 pt, so nothing fits even at the default text size.
     • The first aim step "shows" an invisible legend and retires it, so no ring on that phone is ever explained.
     • The same happens at AX sizes, and whenever the first ring sits near a lane edge.
   • Fix: fire onLegendShown from onAppear on the drawn legendText, not on the container.

2. The card and the legend give different step instructions side by side. That is the issue's core complaint, the overlay and the card disagreeing, now in words.

   • Where: ScanCopy.swift:44 (the card: "Take a small step sideways as you look.") and ScanCopy.swift:180 (the legend: "take one step to the side").
   • The PR's own rationale says a small step may leave the ring unchanged: a view is kept after about 0.5 m of moving, and a cell needs a 0.25 m baseline.
   • On the first tilt-down both lines are on screen together, and one tells the homeowner to do what the other implies won't work.
   • The PR rightly flags this as a team call. It is a one-word change in the card (the U4 owner's file), and it should be settled before any demo rather than left open.

Notes

• Earlier must-fix (the green ring never shows): addressed.
  • CameraOverlays.swift:30-45 scores the outgoing step against the new strip.
  • heldTarget (:88) covers the frame drawn before onChange runs.
  • .task(id: completions) clears the hold, and a newer completion cancels it without clearing the new ring.
  • The single TargetMarker slot (WayfindingOverlay.swift:40-41) keeps the ring's identity, so it animates to green.
• The other earlier items are addressed too: the side hysteresis (:84-90), the shared lane constants, the onboarding line covering both kinds of ring, and the "one step" wording.
• The tolerance fix is right, and the ends filter doesn't need mirroring.
  • ScanViewState+AimProgress.swift:38 now matches CoverageMap.indices(overlapping:).
  • publishCoverage builds the strip from map.visibleRange, which is already clipped to the marked ends, so the strip and the planner count the same cells.
  • I found no case where the live ring turns green with its tick while the card still asks for that stretch.
• Ring vs card during the 1 s hold.
  • The card already shows the next step, while marker(in:) (WayfindingOverlay.swift:163-167) replaces the next step's ring or arrow with the green ring at the old spot.
  • Example: after a tilt-down, the card says "Tilt up to show the wall", the only marker is the ticked ring on the ground, and the tilt-up arrow is held back for 1 s.
  • Green with a tick reads as done, so I don't count this as a contradiction. Check it on a phone, and shorten the hold if it confuses.
• Merge with beta: no behaviour lost.
  • Against origin/beta, ScanCopy.swift differs only by the moved aimRingLegend and the new aimRingLegendShort. Beta's aim(_:ground:hint:), the coaching helpers and the #120 card copy are intact.
  • WallWalkScreen.swift is identical to beta.
  • Beta is now at 25f1f45 (#131, #132), and git merge-tree against it is clean, including ScanCopy, DemoEngine (setWindowOpens) and ScreenStatesUITests.
• Accessibility otherwise:
  • The legend and the ring can't be tapped (allowsHitTesting(false)), so hit area doesn't apply.
  • aim.ring keeps its label, its percent value, and "Captured" when done.
  • The arrow stays hidden from VoiceOver.
  • This PR neither improves nor worsens the legend's contrast (Must fix 1). It removes the legend at AX5 (Must fix 2).
• Demo (unfrozen -uiDemoAim), low confidence.
  • The aim tick no longer calls refreshCoverage(), but the next .walk tick does (DemoEngine.swift:447).
  • That rebuilds the ground from the formula, with reachedRight starting at 0.9 and a 0.35 lag. So cells near s 0.55-0.7 can go back to haze while the green ring is still held.
  • Cosmetic and demo-only. Check it on a Simulator recording.
• Swift:
  • The Mac mini device build (Swift 6, strict concurrency) passed.
  • Using the file-private AimSnapshot as a @State type is fine.
  • onChange(of: AimSnapshot) fires on every target move during walk steps. That runs once per frame, but each run is only a filter over the strip.
• Public repo: the diff, the three commit messages and the PR body contain no names, addresses, meter numbers, private/ content or internal thresholds. The numbers quoted are the app's own constants.
• CI:
  • The PR's iOS build check is skipping (draft).
  • The only real iOS run on this branch (36303117885) failed.
  • The PR body's "PASS" leaves out the audit, and the body says so.

Verdict: blocked. Must fix 1: this branch's own run fails the audit, and this head doesn't change that element, so coordinate with followups-2. Must fix 2: at large text sizes the legend is dropped or drawn under the enlarged card, against the lead's explicit ask.

🤖 Generated with Claude Code

[SamGu-NRX]

@AidenJohnston I inspected b2c6367 against the reported legend defect. ViewThatFits plus the shorter wording addresses the unbounded400pt text frame, but the available space still comes from fixed laneTop260/laneBottom300, not the enlarged instruction card. The normal-size contrast styling is unchanged. Please retain both observed cases for validation; source alone does not establish either fixed.

Also, onLegendShown now belongs to the ViewThatFits wrapper, including its Color.clear fallback. When neither label fits, that can record the legend as shown even though no text was displayed. Please mark it shown only when a text choice actually appears, preserving an accessible explanation when the visual legend cannot fit. No additional CI requested.

[SamGu-NRX]

@AidenJohnston Bounded review at 8b42e13: per-choice appearance reporting plus the under-card fallback addresses the previous empty-fallback callback finding in source. One measured-bound placement defect remains in WayfindingOverlay.swift:103-114,138-142.

The chosen interval still starts at the ring edge, even when that edge is underneath the chrome. For example, in an 844pt view, card bottom400, controls top600, on-screen ring y250/radius30: measured lane is412...588, roomBelow308, reach296. The legend is top-aligned at292, so a text block that fits is drawn behind the card ending400 instead of using the under-card fallback. The ring qualifies as on-screen because placement(for:) still accepts y150...664. The symmetric case puts an above-ring legend behind bottom controls.

Intersect each candidate interval with the measured open area before calculating its capacity and position, or select the card fallback when the ring-adjacent interval cannot remain inside those bounds. Add coverage for a ring under the top card and over the controls; current centered fixture only checks one pose.

For combined pickup with #137 at5c83ad49efd108874102b75327798ecd4d654f82, retain this branch's fitting/callback/measurement implementation and apply Palette.chalk + ScrimShape.rounded(14) inside legendText. The card fallback already uses that opaque scrim. This is a bounded moving-target layout follow-up, not evidence of a capture failure or a reason to block assembly of a controlled combined runtime candidate. No runtime pass claimed.

[SamGu-NRX]

@AidenJohnston Combined beta 6bc1df0, hosted36306893657, supplies runtime evidence for the remaining measured-lane finding5854227050.

The AX5 non-overlap assertion is still valid. instruction identifies only InstructionCard's message (InstructionCard.swift:104-138); CameraChrome.swift:62-67 puts the fallback legend in a sibling below that card. It does not put the legend inside the instruction. Test488 observed legend(37,486,284,248) overlapping instruction(34,148,334,638). Keep the assertion. The source defect is WayfindingOverlay.swift:101-115,134-137: below starts at the ring edge without clamping to measured laneTop; above ends at the ring edge without clamping to laneBottom. When the scrollable AX5 card extends below the ring and pushes controls downward, roomBelow can stay positive and ViewThatFits accepts a legend within the card. Intersect both candidate intervals with the measured open area, then position from that intersection; use the chrome fallback when neither fits. Preserve the opaque137scrim.

A second persistent failure at test564 is normal wallWalk-aim: Dynamic Type font sizes partially unsupported on aim.legend. This is not established as a fixed-font bug: the overlay uses semantic subheadline and fallback uses semantic body. The supplied log cannot identify the internal audit cause. The layout replaces full text with short text or a separate fallback node as sizes change, all sharing aim.legend; that lifecycle is a candidate cause, not a verified diagnosis. Please record the selected placement, content-size category, rendered font/text bounds and number of legend elements across size changes after repairing the intervals. Verify scaling, complete text, single accessibility representation and scroll reachability, then retain the full audit. Do not suppress dynamicType, cap its size or remove the legend to pass it.

Parent plus fresh independent source review; no new CI or source changes from this caretaker. The 6bc run no longer reports the old aim.legend contrast failure, but these two failures prevent claiming aim accessibility accepted.