[AidenJohnston]

What changed

Nine build 4.1 fix PRs were merged onto beta, some before their review fixes were applied. I checked every Must-fix and Should-fix item from their reviews and the owner's follow-up comments against the current beta code (5bc81fb), including the "repair for beta testing" merges. Three things were missing. This PR fixes them:

• The drift log for AR overlays drift off their objects after walking away (window outline drawn over the panel) #73 now records what happened, so a missing line no longer has to be read as evidence (ios: log meter-anchor corrections so marks can be checked against drift #119). The correction lines are .notice, so they are kept on the device and show in Console without "Include Info Messages". Each line carries the capture clock t and the phone's distance from the meter. A line is also written when frames stop carrying the meter's anchor and when they carry it again. Every phase change after the meter is anchored writes a summary, even with zero corrections: the correction count, frames with and without the anchor, and the latest frame's sighting. Sending the scan and opening the AR result always write one. The code comes from the salvaged diff of the cut-off fix run, reviewed hunk by hunk. It applied cleanly.
• On a LiDAR phone the AR result no longer draws twice when the battery leaves the view (ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100). ResultOverlayPolicy.update takes a new held flag (anchored and enabled, whether or not in view). Seeing the battery on screen is needed only for the AR scene to take over. After that the AR scene keeps the result until the anchor or the model is lost. Before, turning to the meter brought the Canvas back over RealityKit's copy, adding a second, unoccluded cable and tint a few centimetres off.
• A server request now names its whole stretch, with the ends rounded outward (ios: gap-loop follow-ups, upload ticks, the nearer unseen end and request stretches (#39, #31, #83, #75) #109). The card used to clip the stretch to the marked ends and round each end to the nearest foot. The owner asked for the full request and outward rounding. A server request is met only over its whole span, and ground past a limit end is asked for (ios: past-end requests keep their end; no requests past a wall end (#35, #38) #47). So a 4–33 ft request with a limit end at 20 ft read "from 4 ft to 20 ft", and 2.4–7.4 ft read "from 2 ft to 7 ft". In both cases, showing what the card said left the progress bar short. ScanCopy.gap loses its ends: parameter, and both callers are updated.

Review items against beta

PR	Item	On beta (5bc81fb)	Here
#100	S1: going off screen after RealityKit takes over brings the Canvas back (owner asked for a fix)	Not fixed	Fixed here
#100	S2: chevron hidden under the card or buttons	Not fixed: the lane only places the chevron; whether it shows still used the whole view inset by 24 pt	Wrongly marked already fixed here; fixed in #137
#108	S1: the no-raise rule depends on which ground plane ARKit finds first (run 3 case)	Not fixed	Deferred: the owner accepted it as a known limit for the beta; device check
#108	S2: the one-shot re-fit can lock onto a small early wall fragment	Not fixed	Deferred: the owner accepted it as a known limit for the beta
#108	S3: a lower ground can still un-meet cells (#62)	Not fixed	Deferred: stays open under #62
#108	S: the rest of #69 (export the disagreement, mid-walk re-fit) has no tracker	Not tracked	Deferred: needs an issue, no code
#102	M1: coachingHidesReply conflict with f14947e	One exhaustive switch, #104's cases included	Already fixed
#102	S1: the reply slides over new words on a coaching change (#64)	.transaction(value: instruction) on the stack	Already fixed
#120	M1: a stalled lagging-band aim is asked for again at once	Fixed (8487a02: runs split at deferred stretches)	Already fixed
#120	M2: iOS CI red	Green on 8487a02 (run 36301331440)	Already fixed
#120	S1: the aim title flickers at the band edge	Fixed (edgeMargin, axisBias)	Already fixed
#120	S2: the overhead step is effectively never asked	Not changed	Deferred: product decision for the lane owner
#120	Owner P3: needsSecondPosition compares stored sightings across an anchor correction	Not changed	Deferred: P3, no dead end shown
#104	M1 (first review): ride-along coaching replaced the task's second line	Fixed (ScanCopy.withCoaching)	Already fixed
#104	S4 (first review): ride-along coaching overrode the refusal tone	Fixed	Already fixed
#104	S5 (first review): the tray-open filter covered only .slowDown	Fixed (.slowDown, .turnSlowly, .holdSteady)	Already fixed
#104	M1 (second review): coachingHidesReply missing the new cases	Fixed	Already fixed
#104	M2 (second review): a walk-out request counted as aiming	Fixed (asksToWalk)	Already fixed
#104	M3 (second review, also S3 in the first): the gap request hidden while dark	Fixed (ride-along on the gap screen)	Already fixed
#104	S5 (second review): the corner step counted as aiming	Fixed	Already fixed
#104	S6 (second review): live card.label reads	Fixed	Already fixed
#104	Owner: keep #109's accessibility-size "Show my result" placement in GapRequestScreen	Both kept	Already fixed
#104	S4 (second review): blur coaching can't show in practice	Documented only	Deferred: product call on the blur rule
#109	M1: gapRequest-followUp-AX5 contrast	Fixed (a44780a: below the tape at AX sizes)	Already fixed
#109	S: the .exists check should check that the control can be tapped	Fixed (scrollUntilHittable in controls)	Already fixed
#109	S3: clipping at a limit end names too little (owner asked for a fix)	Not fixed	Fixed here
#109	S4: nearest-foot rounding names too little (owner asked for a fix)	Not fixed	Fixed here
#109	S2: a request needing a refused view is raised again	Not changed	Deferred: the owner said it is not required
#109	Note: the demo review sample's unseen end contradicts the demo tape	Not changed	Deferred: review screen, which t3/41-ac-review is working on
#119	M1: .info lines aren't kept, so "no line" can't be trusted	Not fixed	Fixed here
#119	M2: a missing line has other causes (anchor lost)	Not fixed	Fixed here
#119	S1: lines can't be matched to the packet or the video	Not fixed	Fixed here
#119	S2: no test runs the engine's own mark move	Not changed	Deferred: the app target has no unit tests
#99	S1: wallWalk-endQuestionLeavesOut at AX5	In largestTextStates	Already fixed
#99	S2: default: in the onWalkTask switch	Not changed	Deferred: style only, fails safe; t3/41-ends-corner is working in that file
#87	S1: PR body verification out of date	Body only	Deferred: no code

Totals: 6 fixed here, 17 already fixed on beta, 13 open when this merged (12 deferred, plus #100 S2, which this table first marked already fixed and #137 fixes). Items that only asked for a rebase or a PR body edit on the original branches don't apply once the branch is on beta, so they aren't counted.

All nine PRs were reviewed, #120 included (its review was of 6ea58d5). Only the fix commit that landed on beta, 8487a02, had no adversarial pass after it. I reviewed it:

• Deferred cells split the lagging runs, and a run's middle can't land in a deferred stretch.
• The hint hysteresis mapping is exhaustive.
• Previous hints are passed only for the same step.
• "Stall with no switch" is logged correctly.

I found nothing new.

Why

Each of these makes a device run look wrong or leaves the next test unable to answer its question:

• On a LiDAR phone the battery showed twice.
• A ground request's words promised less than the request needed.
• The AR overlays drift off their objects after walking away (window outline drawn over the panel) #73 device walk would have read "no line" as evidence, and the old lines weren't kept on the phone anyway.

Verification

• Mac mini (ci-local.sh t3/41-review-followups followups, which mirrors .github/workflows/ios.yml without the accessibility audit): PASS on d5c403d. That covers the xcodegen drift check, swift test HouseScanKit with warnings as errors, the device build, and the UI tests: 23 run, 1 skipped (the optional recording), 0 failures.
• The every-state accessibility audit is skipped by ci-local.sh. The one screen whose words change here is a server gap request, and the demo request's text is unchanged.
• New package tests: ResultOverlayPolicyTests.confirmedThenOutOfViewButHeldStaysOnRealityKit and heldButNeverDrawnKeepsTheCanvas, and AnchoredMarksTests.anchorPresenceReportsOnlyChanges.
• Distance.range is in the app target, which has no unit-test target, so there is no unit regression for the rounding or the clipping. The existing gapRequest-groundOut UI expectation ("From 4 ft to 7 ft right of your meter.", for 4.27–6.89 ft) still holds under outward rounding.

Needs a device:

• On a LiDAR phone, "See it on your wall": turn to the meter and back. There should be one battery and one cable. Attach the AR result drawn by log lines.
• On a walk away from the meter and back, Console without info messages should show meter anchor in frames: and meter anchor at <phase> -> <phase> lines, and meter anchor corrected at t= lines if ARKit corrected the map.
• A server ground request past a limit end should name the whole stretch.

Risk

• Overlap with parallel branches:
  • ScanEngine.swift: go(), ingest, both resets, setMeterAnchor and watchResultInCamera. t3/41-walk-exit also works in ScanEngine, but these edits add lines only and are away from cannotAccessArea and the walk's end.
  • GapRequestScreen.swift and ScanCopy.gap: one line each. A branch that still passes ends: to ScanCopy.gap won't compile until it drops the argument.
• The drift log writes a .notice line on every phase change and on each anchor present/missing change. That is low volume, but a flickering anchor would write a line per change.
• Naming the whole stretch past a limit end can ask for ground past something the homeowner marked as blocking. That is what the request needs to be met (ios: past-end requests keep their end; no requests past a wall end (#35, #38) #47). "I can't get there" and "Show my result" remain the way out.

Part of #41. Part of #67. Part of #73. Part of #75.

Checklist

• The scope is focused; related changes belong together
• I explained what changed and why
• N/A before/after images: the one visible change is copy on a server request card, which the demo's request doesn't change, and there is no Simulator here to capture it
• N/A video: no motion change on screen

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 7ee490d9-0209-490d-9769-0a6bdd5c2439

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.}

[SamGu-NRX]

@AidenJohnston Bounded independent review at d5c403d found no actionable source issue. The full-span/outward-rounding change resolves my #109 range-copy finding in this branch. Both changed ScanCopy.gap callers match the new interface. Fresh read-only review traced held/drawn ownership through tracking loss and the result view, plus anchor presence/correction logging and resets.

Validation remains incomplete: current-head iOS is skipped, the body still contains RESULT_PLACEHOLDER, and the named local script excludes the every-state accessibility audit. Actual LiDAR rendering/occlusion, persisted device logs and the full ground request beyond an end remain unverified. The range transformation also lacks an isolated regression; please cover positive, negative and meter-crossing spans when adding test coverage. No new run requested and no hold on the existing f35 candidate.

[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.

[AidenJohnston]

Adversarial review of #132 at d5c403d (base beta 5bc81fb, which is also the merge base). I read the diff and the changed files on the branch, origin/beta, and the review threads on #100, #102, #104, #108, #109, #119 and #120.

Must fix

None.

Should fix

1. ios/HouseScan/Runtime/ScanEngine.swift:414 and :1453, with ResultOverlayPolicy.swift:34: a tracking blip or a model rebuild still brings the double draw back while the phone points at the meter. The latch holds only while held stays true without a break. Two things in the engine make held false for at least one 100 ms poll:

   • Any tracking change away from .normal calls setResultVisible(false), so isEnabledInHierarchy is false.
   • A rebuild when the wall moved more than 5 cm (showResultInCamera(rising: false)) calls removeResult() and then adds a new AnchorEntity, which may not be anchored on the next poll.

   Either one resets usesRealityKit, and taking over again requires drawn, which means the battery's middle must be on screen.

   • Scenario: RealityKit is confirmed on a LiDAR phone. The homeowner swings the phone quickly to the meter, and ARKit reports .limited(.excessiveMotion) for about 200 ms. Alternatively, ARKit corrects the meter anchor by more than 5 cm as they walk back. When tracking returns, RealityKit is enabled again, but the policy is back on the Canvas. With the spot off screen, the second, unoccluded cable and tint return near the meter, which is the #100 S1 case. They stay until the battery is back in view for 0.5 s.
   • The new tests cover only an unbroken held.
   • Suggested fix: keep a "confirmed once this AR session" flag in the policy. After a loss of held, take over again once held has been true for confirmation seconds, without onScreen. #67 isn't at risk, because non-LiDAR phones never reach this path. Also add a test for "confirmed, held drops for one poll, held returns off screen".

2. The table's "#100 S2: Already fixed (chevron lane 260/300 pt)" claim is wrong. The finding is still open on beta (ios/HouseScan/UI/Screens/ResultARScreen.swift:128-129, unchanged here). The lane moved where the chevron is drawn. The decision whether to draw it still uses the whole view inset by 24 pt: view.contains(point) returns nil when the spot is on screen.

   • Scenario: at the demo wall the presenter holds the phone up toward the meter, and the battery's spot projects under the bottom 300 pt of buttons or the top 260 pt card. No chevron shows, and the battery is behind the chrome.
   • Fix: test visibility against lane (or the same CGRect(x: 36, y: 150, …) the walk uses) instead of view. Until then, change the table row to "Not fixed, deferred".

3. This PR's GitHub run can't pass the merge gate as things stand.

   • The body says the Mac mini ci-local.sh run skips the every-state accessibility audit.
   • The full iOS run on beta's own head (5bc81fb, run 36302099002) fails that audit at ScreenStatesUITests.swift:503: wallWalk-aim: Contrast failed … id 'aim.legend'. aim.legend came from 9704867, #117's aim-ring change, not from anything here.
   • Expect the queued PR run 36304559523 to fail the same way. Say so in the Verification section, and link the fix or the owner's waiver rather than reporting "passes CI".
   • The same run shows gapRequest-followUp-AX5 passing on beta, so #109 M1 really is fixed.

Notes

• What this PR fixes (check 1).

  • #100 S1: fixed for the reported case, turning to the meter without a tracking break. held gates keeping RealityKit but never taking over (heldButNeverDrawnKeepsTheCanvas), and the held default of false keeps other callers on the old behaviour. The Canvas still appears whenever RealityKit hasn't been confirmed, so the #67 fallback still works. The chevron (SpotDirection) doesn't depend on resultInCamera, so it still shows while RealityKit holds the result.

  • #119 M1, M2, S1: all three are addressed. The lines are now .notice. Anchor present/missing/other-anchor lines are written on change only. A summary is written on every phase change once the meter is anchored, and always on .uploading and .resultAR. t and the distance from the meter are on each line.

  • #109 S3, S4: fixed. Distance.range names the full span and rounds outward, and I checked the arithmetic by hand:

    Request	Card text
    2.4…7.4 ft	2–8 ft right
    −7.4…−2.4 ft	2–8 ft left
    −2.6…1.2 ft	3 ft left to 2 ft right
    4.27…6.89 ft (demo)	4–7 ft
    0…x ft	"from your meter"

    With outward rounding, (0, 0) "around your meter" can no longer be reached in practice. That is harmless.

  • The only other ScanCopy.gap caller is ScanEngine+GuidanceLog.swift:42, which is updated.

• Beta claims I spot-checked (check 2), all confirmed on origin/beta.
  • #120 M1: 8487a02 is an ancestor. laggingBand's nearestRun splits runs at deferred cells (GuidancePlanner.swift:389-401).
  • #102 M1 / #104 M1 (second review): coachingHidesReply is exhaustive with the #104 cases and has no default (WallWalkScreen.swift:287-291).
  • #102 S1: .transaction(value: instruction) is at InstructionCard.swift:79.
  • #104 M2: asksToWalk is used in isAiming (ScanEngine.swift:740) and tested.
  • #104 M3: GapRequestScreen.swift:130-135 uses the ride-along and coachingReplacesTask.
  • #104 M1 (first review): withCoaching keeps task.detail (ScanCopy.swift:173-176).
  • #109 M1: the AX-size layout is there, and the AX5 audit passed in run 36302099002.
  • The only wrong claim is #100 S2 (Should fix 2).
• Deferred items against the 5-minute judge demo (check 3). None has to block this PR.
  • The one I would not leave to chance before the TestFlight is #108 S2 (MeterTap.swift:125). refit accepts a .wall plane of any size, once. An early 0.3 m fragment 11° off turns the whole wall, and the spot and clearances turn with it.
  • #108 S1 (GroundPlaneChoice.swift:108) depends on the site: it only matters if the demo wall has two ground levels.
  • Both need one device run at the actual demo wall, checking the refit and ground-choice log lines, before the build is used for the demo. A minimum plane extent in refit would be a cheap guard.
  • #120 S2 (overhead never asked on the walk) costs one extra server "Tilt up here" loop per scan, which is time in a 5-minute demo but not a failure.
  • #109 S2 and #109's demo-sample note don't affect a live-phone demo. reviewSample is DemoEngine-only.
• Regressions in the new code (check 4).
  • Logging: there's no per-frame spam. noteMeterAnchor runs on every delivered frame but logs only on a change. Pose-only and sampled frames build meterAnchor the same way (LiveCapture.swift:441), so they can't alternate. Expect 2 or 3 lines at anchoring (a queued frame with the old id, then missing, then present), plus one per phase change.
  • logAnchorCorrection is now a persisted .notice on every applied correction, pose-only frames included. If a non-LiDAR anchor jitters by more than 2 cm, that becomes one persisted line per pose frame. That's acceptable for a diagnostic, but it's worth a one-line rate cap if device logs show it.
  • No private data is logged: only floats, ints, the phase and fixed strings. t is the ARFrame uptime clock.
  • LiveCapture.swift:173 computes inSession but held (:186) doesn't use it. Adding inSession && to held costs nothing and guards against an AnchorEntity that stays isAnchored after ARKit drops the meter anchor.
• Public-repo rules (check 5). The diff, the three commit messages and the PR body contain no names, addresses, meter numbers, private/ content, internal thresholds or media. The numbers in comments (2.4…7.4 ft, 4–33 ft) are synthetic or come from public issues.
• Overlap. t3/41-ac-review, t3/41-ends-corner and t3/41-walk-exit each still contain the two ScanCopy.gap(gap, ends:) calls, but they are beta's own lines, unchanged on those branches. They merge cleanly after this PR, and only a newly added caller would break the build.

Verdict: ready-with-notes

🤖 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.