Repository navigation
ios: make beta green: aim legend layout and review-flow test waits - #155
Conversation
Beta's aim legend, from #117 at an older head, drew beside the ring and moved under the card only when no wording fit there. The full audit on beta 6bc1df0 failed both ways: at the largest text size the beside-the-ring legend landed on the card, and at the default size the audit reported its Dynamic Type as partially supported. This takes the layout from t3/41-target-ring (23cd43e): the legend always sits under the instruction card in CameraChrome's stack while the first filling ring is on screen, so it grows and scrolls with the card and never overlaps it. The measured open area, the side hysteresis and the short wording go. The line is now one Text with the symbol inline rather than a Label with fixedSize, the form the result's sample badge moved to for the same audit finding. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since d4e008c the ScreenStatesUITests tap helper waits for isHittable before tapping. A control below the bottom edge of a scrolling screen never becomes hittable where it is; XCUIElement.tap() used to scroll to it, the wait doesn't. On beta 6bc1df0 that failed testAddSomethingFromTheReviewMarksOnTheCamera (the review's "Add AC unit" sits at y 1128 on an 874 pt screen), testWholeFlowThroughTheButtons and the audit's result states (the result's Details sits under the diorama, answer and footnotes). The helper still waits for the control to settle first. If after 3 s it isn't hittable and lies past the bottom edge, it drags the screen up with the audit's slow drag until it is, then waits out the rest of the timeout. The aim ring test's comments follow the legend's new place (23cd43e). 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 |
|
@AidenJohnston Root has read c8dd2f8 and has made no overlapping test edits. It will use your ScreenStates tap repair after this bounded source review. Its separate beta-integration test changes cover Practice result Details expansion, failure-safe switch cleanup, explicit practice-off for ordinary ScreenStates launches, and a dedicated Practice-on onboarding default/AX5 audit so isolation cannot mask that real Dynamic Type finding. Existing assertions stay. Aim source ownership remains yours; no duplicate CI is being launched by this caretaker. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c8dd2f81d2
ℹ️ 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".
| if let legend { | ||
| legendLine(legend) |
There was a problem hiding this comment.
When an aim target projects near the top of the screen, this legend is now painted above the camera overlay and can cover the ring it is supposed to explain. WayfindingOverlay.placement still treats any center from y = 150 downward as an on-screen marker, while the newly stacked card and legend can extend well below that—especially at accessibility text sizes—so the marker emits no edge arrow and onFillingRingShown reports it as visible even when the legend obscures it. Account for the legend's actual lower edge when deciding whether the marker is on screen, or otherwise keep the legend from covering that marker region.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The filling ring still uses a fixed 150-point top exclusion while card/legend can extend below it. Feed the actual stack bottom into marker visibility and edge-arrow placement so an obscured ring is not reported visible. Check default and AX text sizes with the ring under the legend. Evidence on alpha: ios/HouseScan/UI/Camera/WayfindingOverlay.swift:187; ios/HouseScan/UI/Components/CameraChrome.swift:106. This remains open in the triage for #237.
|
@AidenJohnston Validated Codex finding4114898322 at c8dd2f8. WallWalkScreen paints CameraChrome after CameraOverlays; WayfindingOverlay.swift108-112 still classifies centers from y150 as visible without measuring card/legend occlusion, and37-41 reports that ring as shown. A target under the newly stacked legend therefore gets neither a visible ring nor the offscreen arrow. This is a P2 moving-target/large-text follow-up, separate from the old legend-over-card defect this commit fixes. Account for the actual visible camera region when placing ring and arrow, and avoid feedback flicker if hiding the ring also removes the legend and changes that region. Keep the full text and existing assertions. Combined runtime test need not wait for the bot request; this is a completed review with a validated finding, not a clean verdict. |
The first full run on c8dd2f8 passed the review and whole-flow tests, but the audit's result-review-AX5 state still couldn't reach Details: eight slow drags didn't make it hittable, while the plain tap that followed scrolled to it and opened it. A control that isn't hittable after 3 s and lies past the bottom edge is now tapped where it is, and XCUIElement.tap() scrolls it into view, as it did before d4e008c. A control on screen still waits until it is hittable. The drag helper goes back to its four drags. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
AidenJohnston
left a comment
AidenJohnston
left a comment
There was a problem hiding this comment.
Fresh review of af1914e against beta (base 6bc1df0; it merges cleanly onto the current beta 7b85a39, and nothing in the merged tree still uses the removed openArea, OpenCameraArea, LegendPlacement or aimRingLegendShort).
(B) Tap helper: the d4e008c protection is kept where it matters, with a narrow exception.
- A control that is on screen, or above the window, still has to become
isHittablewithin the full timeout, or the test fails. That covers the case d4e008c was written for ("Mark something" still moving into place after the close-up). The helper waits up to 3 s, then waits out the rest of the timeout. - The exception applies only when a control still isn't hittable after 3 s and its frame ends below the window. Then the helper calls
tap()without checking. It doesn't just drop the check:XCUIElement.tap()scrolls the element into view and works out a hit point first, and it waits for the app to go idle. So a missing or unreachable control still fails. It doesn't let a broken screen pass. - It is weaker than "scroll into view, then require hittable" in one way: after the scroll, nothing checks that the control has stopped moving before the tap. For the controls this is for (the review's "Add something" chips and the result's Details, below a static scroll view), that's fine. The PR body says drags didn't reach Details at AX5, which is a fair reason not to scroll by hand.
- Not a must-fix. Optional hardening: after
tap()on the below-the-fold path, check the tap's effect where the call site allows. Most call sites already do this with the nextwaitForExistence.
(A) Legend under the card: correct, and never hidden.
- The code matches 23cd43e plus the chalk/solid-scrim colour already on beta. The legend line is one
Textwith the symbol inline andaccessibilityLabel(text), so VoiceOver reads the words without the symbol name. - It sits in
CameraChrome's stack inside the existingScrollView, withfixedSize(vertical). At AX5, or on a small phone, it grows and scrolls with the card and can't overlap it. The only conditions that hide it are the ring conditions (a filling ring on screen, first step, not marking, not held). It no longer counts as shown when it isn't drawn:legendStepis set only whencardLegendis bound. The AX5 test checks that the legend exists and doesn't intersect the card. - Note 1: while it shows, the legend takes camera space right under the card. A ring held high in the frame can now sit behind the legend it explains, which the old beside-the-ring layout avoided. It's short-lived (first ring only), so I'd accept it and watch for it on device.
- Note 2 (app behaviour change):
GapRequestScreenpasses nocardLegend. On beta, itsCameraOverlaysstill drew the legend beside a filling ring. With this PR the gap screen never shows the legend. That seems in line with #81 ("explain the first ring once"), but it's a change, so it should be stated as intended. - Note 3:
.animation(.easeOut, value: legend)on the whole stack only fades the line in and out (opacity transition), which is fine under Reduce Motion. At AX sizes, the scroll content height changes when the ring comes and goes near the screen edge, which may look jumpy on device.
Swift / tests. onFillingRingShown uses a zero-size Color.clear with onAppear/onDisappear. It reports false when the overlay is removed for marking, and shownLegend guards on marking as well, so the flag can't stay stale. onChange(of: shownLegend, initial: true) writes the binding only when the value changes. No app code changed in (B).
Public-repo rules. Nothing from private/: no prompt text, internal field names or internal thresholds, in the diff or the PR body.
The two remaining audit failures: I agree neither comes from this PR.
FullFlowUITests.testPracticeMeterFromReplayisn't touched, and it doesn't use the changed helper (it lives in another file and uses its own taps). It turns the switch on in onboarding and callssetPracticeMeter(false)only after the assertions and theaction.startOvertap. When those fail, the switch stays on in UserDefaults for the rest of the Simulator run. That explains the audit's onboarding "Practice meter on" Dynamic Type item. Fix it on the practice-meter side: reset the switch in teardown, or launch the audit with the switch off.markFeatures-groundAnsweredcontrast "(no element)": this PR changes nothing on that screen or in the shared palette or scrim, and the aim legend is only wired onWallWalkScreen. An item with no element that shows up intermittently looks like a transient over the replay's camera image, not something this PR caused.
Verdict: ready-with-notes
🤖 Generated with Claude Code
|
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 Screenshot evidence for existing marker-occlusion follow-up4114898322, from hosted run36310019572 at4482ab1cb0bf1352311479778e9313ca39befe22: wallWalk-aim.png shows the legend separated from the instruction card and a visible ring. wallWalk-aim-AX5.png shows the enlarged instruction card occupying almost the entire viewport, with no visible ring, legend or map. I inspected both images. This supports the retained large-text visibility issue after the legend-layout repair, not a new issue or an ordinary-demo hold. It does not establish how scrolling behaves on a phone. The images are in that run's UI screenshots evidence: https://github.com/SamGu-NRX/house-scanning-master/actions/runs/36310019572 . Local extracted copies are beta-qa/evidence/candidate-4482ab1-screens/wallWalk-aim.png and wallWalk-aim-AX5.png in the QA workspace. Preserve4482 as their source revision; these are not screenshots of current beta2db or the still-running5532 candidate. |
2 participants
Beta 6bc1df0 is red on the full iOS run 36306893657. This fixes the two failures that come from our build-4.1 work and leaves the practice-meter failure to its owner.
Fixed
A.
testAimRingShowsProgressAndItsLegendand the audit'swallWalk-aimitem. Beta has #117's aim legend from an older head: it draws beside the ring and moves under the card only when no wording fits there. At the largest text size it landed on the card, and at the default size the audit reported its Dynamic Type as partially supported. This takes the layout fromt3/41-target-ring(23cd43e). The legend always sits under the instruction card, inCameraChrome's stack, while the first filling ring is on screen, so it grows and scrolls with the card and can't overlap it. The line is oneTextwith the symbol inline, not aLabelwithfixedSize, the same change the result's sample badge needed for the same audit finding.B.
testAddSomethingFromTheReviewMarksOnTheCamera,testWholeFlowThroughTheButtons, and the audit'sresultstates (all atScreenStatesUITests.swift:620). None of the suspects caused this. The soft gate (#131), the corner confirm (#136) and the practice meter all behave as the tests expect: the whole-flow test answers both questions and reaches the result. The cause is the tap helper change in d4e008c. The helper now waits forisHittablebefore it taps, and a control below the bottom edge of a scrolling screen never becomes hittable where it is.XCUIElement.tap()used to scroll to it, and the wait doesn't. The review's "Add AC unit" sits at y 1128 on an 874 pt screen, and the result's Details sits under the diorama, the answer and the footnotes. The helper still waits up to 3 s for a control to settle. If the control still isn't hittable and lies past the bottom edge, the helper taps it where it is, andtap()scrolls it into view, as it did before d4e008c. A control on screen still waits until it is hittable. The app is unchanged. (A first try dragged the screen to the control instead, but at AX5 eight drags didn't reach the result's Details.)Still red
C.
FullFlowUITests.testPracticeMeterFromReplay. In CI it can't findaction.startOver. On the Mac mini it fails earlier, because the reader never offered the sample meter's number. This is Sam's practice-meter work (t3/practice-meter), so this PR doesn't touch it. A and B don't depend on it.It has one knock-on effect. The test turns the practice meter switch on and turns it off only at its end, after the failing steps. The switch is stored in UserDefaults, so it stays on for every later test in the same Simulator. That's why the audit reports
onboarding: Dynamic Type font sizes are partially unsupported ... 'Practice meter on': the developer options button swaps its words for the icon withViewThatFits. The item isn't from #117. It goes away once C resets the switch, or once the audit launches with-practiceMeter NO. Until then,testEveryStatePassesTheAccessibilityAuditstays red on that item.Testing
Full audit on the Mac mini (
FULL=1 ./ci-local.sh t3/41-beta-green beta-green):result-review-AX5Details, which af1914e fixes.testLooksCompleteFirstPointsToAnUnansweredQuestionandtestRejectedUploadGoesBackToReviewfailed once: the first "Looks complete" tap didn't register. Both passed on the next run and on every earlier Mac mini run.markFeatures-groundAnswered: Contrast nearly passed (no element). That contrast item has no element and didn't appear in the first run or in CI run 36306893657, so it looks like a transient.🤖 Generated with Claude Code