[AidenJohnston]

What changed

The review of #132 found three things still wrong on beta (25f1f45). This PR fixes them:

• The second AR battery could still come back (ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100). Once RealityKit had let go of the result, it had to see the battery on screen again before it took over from the Canvas. A lost hold happens when:

  • the model is rebuilt because the wall moved more than 5 cm (ScanEngine.showResultInCamera), or
  • tracking drops for a moment (ingest disables the model).

  If that happened while the phone pointed at the meter, the Canvas's unoccluded battery and cable stayed on top of RealityKit's copy for as long as the phone stayed there. ResultOverlayPolicy now remembers the first take-over and takes the result back as soon as the AR scene holds it again, with no need for the battery in view. While tracking is limited, the engine counts the disabled model as held. The screen draws neither layer then, so both come back together.

• The AR result's chevron is now shown for a spot hidden under the card or the Done button (ios: draw the AR result with the Canvas until RealityKit is seen drawing it #100 S2). Before, the chevron hid whenever the spot projected anywhere inside a 24 pt inset of the screen. Now "in view" means the band the chrome leaves clear: from 260 pt down to 160 pt above the bottom. In the demo's resultAR state the spot sits inside that band, so that state is unchanged. The chevron's words now sit on the card's solid scrim.

• The every-state accessibility audit failed on beta (run 36302099002). The failure was wallWalk-aim, "Contrast failed" on aim.legend. The legend's background was 70% black, so the demo's bright siding showed through. The legend now uses the instruction card's solid scrim and text colour, which pass the audit on every screen.

#132's body wrongly marked #100 S2 as already fixed. I corrected its table.

Verification

• Mac mini, full UI run including testEveryStatePassesTheAccessibilityAudit (FULL=1 ci-local.sh t3/41-review-followups-2 followups-2): PASS on 5c83ad4. Package tests ran with warnings as errors, the device build passed, and 25 UI tests ran with 1 skipped and 0 failures. The every-state audit passed in 475 s. The first attempt at the same SHA also passed the audit, but failed AuditIssueKeyTests.testAGoneElementIsReadWithoutRecordingAFailure. The tap on "Gravel" didn't register within 5 s. That test doesn't touch any code this PR changes, and it passed on the rerun and in GitHub run 36302099002, so it looks like a tap-timing flake.
• New package test: ResultOverlayPolicyTests.afterATakeOverHoldingAgainTakesOverWithoutTheBatteryInView. confirmedThenOutOfViewButHeldStaysOnRealityKit no longer checks the re-confirmation, which is the behaviour this PR removes.
• I found the audit's cause from the failed run's screenshot of wallWalk-aim: the legend sat over the cream siding.

Needs a device (LiDAR phone), on "See it on your wall":

• Point at the meter with the battery out of view, then cover the camera for a moment. When tracking returns there should be one cable, not two.
• Point the phone so the spot is under the card. The chevron should show.

Risk

• A rebuilt model's new anchor entity may not report anchored until RealityKit's next update. If the watcher looks in that frame, the Canvas can show for one look (100 ms) before the AR scene takes the result back.
• The clear band's 260 pt and 160 pt are layout guesses. A result card with a three-line detail reaches about 250 pt.
• Overlap: ScanEngine.watchResultInCamera and LiveCapture.resultIsDrawn only, both on the AR result path.

Part of #41. Part of #67.

Checklist

• The scope is focused; related changes belong together
• I explained what changed and why
• N/A before/after images: the legend and chevron only change background colour (70% black to the card's scrim), and the Simulator runs here are headless
• N/A video: no motion change

🤖 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 #137 at 5c83ad4. #137 and #117 were both marked merged at 08:51 today. beta (6bc1df0) already contains 5c83ad4 through 50a2953 and #117's 8b42e13 through 7724c42, so everything below is a follow-up for beta, not a gate on this branch. I read the diff, the files it touches on the branch and on beta, #132's review and #67.

Must fix

None.

Should fix

1. ResultARScreen.swift chevronPlacement: the 260 pt / 160 pt clear band is only right at the default text size. beta now has the measured edges, so use them.
   • Measured in full-screen points (SpotDirection ignores the safe area):
     • Card bottom, Dynamic Island phones: top inset about 59, plus 4 pt padding, a 36 pt status row and a 10 pt gap, plus the card. That comes to about 215–235 pt with a two-line detail.
     • A three-line detail reaches about 250–265 pt on a 375 pt-wide notched phone (13 mini, top inset 50). The sample-spot text ("No server checked this scan. …") is the likeliest to wrap to three lines.
     • Done button top: about 98 pt up on home-indicator phones and about 64 pt on an SE, so 160 pt leaves 60 pt or more to spare there.
   • Large text breaks the 260 pt top. From xxxLarge up, the card passes 260 pt. At AX sizes it can reach 400–700 pt, or fill the screen and scroll. Two failures follow:
     • A spot under the enlarged card still counts as "in view", so no chevron shows. This is the #100 S2 bug again, for larger text.
     • The chevron's lane also starts at 260 pt, and SpotDirection sits under CameraChrome in the ZStack. When the chevron does show, the card covers both the arrow and its words.
   • This is the problem #117 solved for the aim legend. CameraChrome(openArea:) reports the card's real bottom and the controls' real top.
   • Suggested fix:
     • Give ResultARScreen an @State var openArea: OpenCameraArea? and pass openArea: $openArea to CameraChrome.
     • Build both clear and lane from it, converting from global coordinates as WayfindingOverlay.legendView does, and keep 260/160 and 260/300 as the fallback when it's nil.
     • When top >= bottom (the chrome fills the screen), no chevron can be seen. Put "Your battery spot is off screen" into the card's legend: line instead, as the aim legend's .underCard does.

Notes

• Can (1) fail the other way (#67)? No. I walked the state machine (confirmed, usesRealityKit, drawnSince; in the engine drawn implies held):
  • Before the first take-over: held alone never takes over (the first branch needs usesRealityKit || confirmed). The Canvas shows until drawn has held for 0.5 s, so the heldButNeverDrawnKeepsTheCanvas guarantee is intact.
  • After a take-over, any break in the chain gives the Canvas back on the next look (≤100 ms):
    • the anchor entity unanchors, so anchored is false;
    • the result is removed (resultIsDrawn returns false for all three);
    • the model is disabled while tracking is .normal.
    • I checked two places where this could fail. The rebuild path (showResultInCamera(rising: false)) keeps the same watcher, so confirmed survives a rebuild as intended. ingest sets state.tracking and calls setResultVisible in the same main-actor turn, so the watcher never sees "normal, but disabled".
  • Long limited tracking: the new anchored && tracking != .normal term keeps resultInCamera true. ResultARScreen draws neither layer then and shows "Point at your meter", as it did before, so this is no worse.
  • What confirmed gives up: after a break, the result goes back to RealityKit without the re-check that drawn used to give. That check was only the same chain plus "the focus projects on screen", and a rebuild uses the same pipeline that was already confirmed on this phone. The cost is small.
  • The failure no version can see, "chain OK but nothing rendered", is unchanged.
  • Non-LiDAR phones never reach this path (showResultInCamera guards on supportsMesh). They always get the Canvas, which was #67's case.
• Cheap hardening, repeated from #132's review: held and anchored still ignore inSession. With confirmed sticky, an AnchorEntity that stays isAnchored after ARKit drops or replaces the meter anchor would now retake at once, with no drawn check. anchored && inSession in LiveCapture.resultIsDrawn costs nothing.
• Flicker at the band edges: the in-view test has no hysteresis. A spot resting on the card's bottom edge, which is where it sits when the phone tilts up toward the meter, will toggle the chevron and its words with hand jitter. #117 added a 40 pt switch margin to the legend for the same reason, and a similar margin here would stop the blink.
• Rebuild flash: as the Risk section says, a rebuilt model's new anchor can be unanchored for one look, which shows the Canvas copy alone for about 100 ms. That's acceptable, and it's one battery, not two.
• Test gaps:
  • No UI or unit test asserts ar.spotDirection either way. The claim that "the demo resultAR state is unchanged" rests on where the demo spot happens to project.
  • Because the chevron never shows in any audited state, the audit has never checked its words' new capsule scrim.
  • Moving chevronPlacement into a pure function (it already takes a size and a projection) would make both testable.
  • afterATakeOverHoldingAgainTakesOverWithoutTheBatteryInView covers the policy. The engine's tracking term has no test.
• Merge with #117 (the contrast fix): 7724c42 resolved it correctly.
  • #117's legendText, inside the ViewThatFits and measured-openArea layout, now carries this PR's Palette.chalk on ScrimShape.rounded(14).
  • #117's under-card legend (CameraChrome.legendLine) already used the same scrim and chalk, so both aim.legend presentations share one surface.
  • They still differ in type: subheadline semibold, centred, beside the ring; Typeface.hint with a scope icon under the card. That's cosmetic.
  • ViewThatFits sizing is unaffected, because the padding didn't change.
  • The solid box is heavier over the wall next to the ring than 70% black was, which is the price of passing the audit.
  • The t3/41-target-ring head still has the old 70% black. Don't re-merge that branch over beta.
• Verification on the combined head: the Mac mini PASS was on 5c83ad4 alone, before #117's layout and the practice-meter work joined it on beta. GitHub's iOS job was skipped on this draft. The first every-state audit of the combination is iOS run 36306893657 on 6bc1df0, still in progress. Check it before the next TestFlight.
• Demo risk: low. On a LiDAR demo phone, (1) removes the only remaining double-cable path I can find, and (2) adds a chevron in a case that showed nothing before. Before the demo, run the two device checks listed in the body. Also check once at a larger text size if the demo phone uses one.
• Swift: if held, usesRealityKit || confirmed binds as intended. The tuple change in resultIsDrawn has one caller. drawnSince can go stale while the first branch returns, but that's harmless: the first drawn == false resets it, and in the engine drawn implies held.
• Public-repo rules: the diff, the three commit messages and the PR body contain no names, addresses, meter readings, private/ material or internal thresholds. The "cream siding" is the bundled demo image.

Verdict: ready-with-notes

🤖 Generated with Claude Code