Repository navigation
fix(ios): the result card names the review line, draws no unsized zones, and promises a photo only when it offers one (on #191) - #209
Conversation
The sweep's start_ft is a range of battery left edges, not a footprint, and the answer sizes a battery only on spot or nearest_considered. With neither, the app drew each run with a width of 0 and a made-up 0.3 m depth. ResultMarkLayout.sweepZones now gives no zones without a size, and gives the same zones as before with one. The wall, summary and reasons stay. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r's review The answer's optional review_threshold_ft was dropped at decoding, so a run of 16 ft read "The rule is at most 20 ft" under "Not sure yet", with nothing about the 15 ft line that made it unsure. PlacementCheck now decodes the line and writes it back only when present, CheckRow carries it, and the measurement sentence names it when ResultReading.reviewBandApplies says it explains an unsure outcome. The card line and its spoken value come from the same function. The sentence says what the rule needs and never that a review was sent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… spot ResultCardUITests reads review-band, unsure-view and no-spot-no-nearest from Fixtures/results through the demo. It checks the cable run line's spoken sentence, that a check without a review line keeps its old sentence, and that an answer with neither spot shows no spot, no check lines and its summary under Details. The review band and the no-spot answer run at the default and the largest text size, with screenshots kept. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ScanCopy decided it from CheckRow's Float meters, and narrowing can move a value across the line: 14.5 ft plus 6 in reaches 15 ft exactly, but the same sum in Float meters fell just under it, so the card would have left the review line out. PlacementCheck.reviewBandApplies now decides on the decoded feet, and the mapping carries the answer on CheckRow. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…or one On the spot-unknown run, "Clear space in front" read "One more photo would settle this" after its request was withdrawn, with no way to take the photo. The note was keyed on needsPerson alone. ResultCardActions.lineOffersView now decides whether a line offers its settling view, and ResultPresentation.offeredView applies it. The line's "Show me", its words on the card and its note under Details all read that one fact. A line with no view to take says "Needs an installer to check". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ard in measured steps Review of d672973: XcodeGen lists every result fixture as a membership exception, so the three new fixtures changed the generated project and CI's drift check failed (run 37117085378). Regenerated with XcodeGen 2.46.0, checksum-verified against ios.yml. `scrollIntoView` swiped up a fixed amount until the line fit. At the largest text size a tall line could jump from below the window to above it, and further swipes up could never bring it back. Each drag now moves the content by what is missing, at most a third of the window, toward the line. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (18)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add review-band data and wording for unsure checks, align camera offers and result-card text with camera availability, and calculate sweep clearance zones from spot dimensions. New fixtures and tests cover these behaviors and results with no selected or nearest spot. ChangesResult presentation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change makes result-card wording, photo offers, and clearance zones consistent with the data available. No concrete merge-blocking risk was identified; the author's hosted test runs passed. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed changes preserve server decisions and existing camera safeguards. No introduced security issue was identified, but the available coverage does not support a complete end-to-end security assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 61.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 14 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
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. |
|
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 BaseScanning to your team's repositories. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard outcome == .unsure, let measured, let threshold, let reviewThreshold, let comparison else { return false } | ||
| let error = max(plusMinus ?? 0, 0) | ||
| switch comparison { | ||
| case .atMost: return reviewThreshold < threshold && !(measured + error < reviewThreshold) | ||
| case .atLeast: return reviewThreshold > threshold && !(measured - error > reviewThreshold) |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/ResultReading.swift
Line: 100-104
Comment:
**Review line misstates uncertainty** When a 14.5 ft cable run has ± 0.5 ft of error, this check treats the 15 ft review line as applicable even though the measured value is not over it. The card then says that “anything over 15 ft needs an installer’s review,” which does not explain why this result is unsure. The same wording appears when the measurement is uncertain near the 20 ft maximum, where the server gives a different reason. Please account for the measurement’s error and the server’s reason when choosing the explanation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
The decision follows the server's rule: the band applies when measured + error is not below the review line, so 14.5 ft ± 6 in is unsure because it can reach 15 ft. With 55fb4b4 the card reads "Measured 14 ft 6 in. The rule is at most 20 ft, and anything 15 ft or more needs an installer's review. The measurement can be off by about 6 in." Together those two sentences are the reason: the run can reach the line. Near the 20 ft maximum the same sentences stay true, and the error sentence carries the margin. The server's own reason is unchanged under Details. The client doesn't switch wording on unsure_cause, since that would mean restating the server's outcome logic on the phone.
There was a problem hiding this comment.
That clarification resolves the concern. The inclusive boundary is consistent with the server/schema rule: at_most only clears the review line when measured + error < reviewThresholdFt, so 14.5 ft ± 0.5 ft correctly reaches the 15 ft line and the review-band wording applies. Reading the values in feet also avoids changing that boundary through Float conversion. The updated “15 ft or more” wording accurately describes the inclusive rule, and keeping the server’s unsure_cause out of the client explanation is reasonable since the client is not recomputing or replacing the server’s outcome. I no longer consider this a valid finding.
| /// Who settles an unsure check. One more photo only when the app offers the camera for it | ||
| /// (`ResultPresentation.offeredView`): keyed on the check alone, a view the homeowner | ||
| /// couldn't get to, or one the app can't plan, still promised a photo nothing would take. | ||
| static func unsureNote(photoOffered: Bool) -> String { | ||
| photoOffered ? Self.onePhoto : Self.needsInstaller |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScan/UI/Copy/ScanCopy.swift
Line: 703-707
Comment:
**Camera failure implies installer needed** If the camera fails after upload, a check that could otherwise be settled by a photo has no offered photo, so this note changes to “Needs an installer to check.” The check does not require a person to judge it, and the headline still says “One more look.” This gives the homeowner a false installer requirement instead of explaining that capture is unavailable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Intended, per the brief this PR implements: a line must not promise that a photo will settle the check when the app won't take that photo. After the camera fails, the result offers no capture for the rest of this session, so the photo line would be a promise nothing can keep. "Needs an installer to check" is the same wording Details already used for views it can't offer, at 75f6c37 and before. It says a check is still needed, not that one was arranged. The headline and the notes come from the server's outcome and aren't recomputed here.
There was a problem hiding this comment.
That’s fair, and the brief makes this behavior intentional: after capture failure, the app no longer offers a photo, so “One more photo would settle this” would be an unfulfillable promise. “Needs an installer to check” describes the remaining action without implying that anyone was contacted or that the server outcome was recomputed. I’m withdrawing this finding.
| XCTAssertEqual(element(app, "result.headline").label, "Not on this wall") | ||
| XCTAssertFalse(element(app, "result.placement").exists) | ||
| XCTAssertFalse(element(app, "result.nearest").exists) | ||
| let checkLines = app.descendants(matching: .any).matching(NSPredicate(format: "identifier BEGINSWITH 'check.'")) | ||
| XCTAssertEqual(checkLines.count, 0, "an answer without checks put check lines on the card") | ||
| let model = app.descendants(matching: .any).matching(NSPredicate(format: "label == '3D view of your wall'")).firstMatch | ||
| XCTAssertTrue(model.waitForExistence(timeout: 10), "the model of the wall is missing") | ||
| XCTAssertEqual(value(model), "No battery spot shown") | ||
| attach(app, name: name) | ||
|
|
||
| let details = element(app, "result.details") | ||
| scrollIntoView(details, in: app) |
There was a problem hiding this comment.
sweepZones alone, so a regression in the export mapping could restore unsized floor zones while both tests still pass. Checking the resulting clearance list would cover the behavior this test names.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanUITests/ResultCardUITests.swift
Line: 119-130
Comment:
**No-zone test misses zones** This test checks that there is no battery spot and that the wall model exists, but it never checks whether clearance zones are present. The separate package test covers `sweepZones` alone, so a regression in the export mapping could restore unsized floor zones while both tests still pass. Checking the resulting clearance list would cover the behavior this test names.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
The 3D scene isn't inspectable from a UI test: RealityKit entities don't appear in the accessibility tree, and the zones have no accessible element. So "no zone drawn" rests on ResultMarkLayout.sweepZones (package tests) and the one-line mapping in ScanEngine+Export that feeds every zone consumer. The PR's Risk section says this. A UI-visible proxy would mean adding an accessibility element only for tests, which I'd rather not ship.
There was a problem hiding this comment.
You're right—the 3D scene and its zone entities aren't exposed through the UI accessibility tree, so this UI test cannot meaningfully verify whether zones are drawn without adding test-only production accessibility surface. Since ResultMarkLayout.sweepZones is covered directly and the single export mapping feeds the 3D, AR, and overlay consumers, the package tests plus mapping coverage are appropriate here. I’m withdrawing this comment; the existing Risk section accurately documents the remaining limitation.
… the result card names the review line, draws no unsized zones, and promises a photo only when it offers one project.pbxproj conflicted only in the generated fixture exception lists (#206's and #209's new result fixtures); it is regenerated with XcodeGen 2.46.0 from the merged tree. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![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: f0c271cfb6
ℹ️ 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".
| case .atMost: "anything over \(length(review))" | ||
| case .atLeast: "anything under \(length(review))" |
There was a problem hiding this comment.
When an at-most measurement plus its error equals reviewThreshold, the server returns UNSURE because passing requires <, and reviewBandApplies also returns true, but this text says only values “over” the threshold need review. The mirrored at-least branch likewise says “under” even though equality also needs review. Use inclusive wording such as “15 ft or more” and “4 ft or less” so the displayed answer matches the rule boundary.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in 55fb4b4. For an at-most check, a confident pass needs measured + error < review_threshold_ft, so a value exactly on the line already needs review. The card now says "anything 15 ft or more" under a maximum and "anything 4 ft or less" over a minimum. reviewBandApplies was already inclusive, so only the words changed.
Review of #209 (Codex): a confident pass needs measured + error < review_threshold_ft for an at-most check (result.schema.json, solver.py reach_outcome), so a run landing exactly on 15 ft already needs review, and "anything over 15 ft" left that case out. The card now says "anything 15 ft or more" under a maximum and "anything 4 ft or less" over a minimum. The decision (`reviewBandApplies`) was already inclusive; only the words change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
… the review line's wording includes the line itself Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
1 participant
Stacked on #191 (
codex/public-assembly-20260930, 75f6c37), alongside #207 and #208. This PR adds 1e45aba through 55fb4b4 (seven commits).What changed
Three places where the result screen said more than the server's answer supports, or less than it needs to. These were findings B-13 and B-14, plus one root found after the spot-check change.
No floor zones without a battery size (B-13)
A sweep run's
start_ftis a range of battery left edges, and the server sizes the battery only onspotornearest_considered(result.schema.json). With neither, the app drew each run 0 m wide past its starts and 0.3 m deep (ScanEngine+Export.swift at 75f6c377), which is a footprint nothing measured.ResultMarkLayout.sweepZonesnow returns no zones without a size. With a spot or nearest spot, its zones are exactly what the old code drew. The 3D view, the AR view and the overlay all draw only those zones, so none of them changed.The card says why a measurement inside the maximum still needs review (B-14)
The server sends
review_threshold_fton a check whose confident limit is tighter than its maximum: a cable run of 16 ft is under the 20 ft maximum and over the 15 ft confident reach (solver.py). The app dropped the field while decoding, so the card read "Measured 16 ft. The rule is at most 20 ft…" under "Not sure yet", with no reason given.CheckRow.PlacementCheck.reviewBandAppliesdecides in the server's feet whether the review line explains this check: an unsure outcome, both thresholds, the line inside the limit, and the measurement past the line in the rule's direction. Any missing field means no.An unsure line promises a photo only when it offers the camera
The unsure line said "One more photo would settle this" whenever no person had to judge the check, even when the line offered no way to take that photo. That happened after the spot check withdrew its front-space request. The line's "Show me" already depended on whether the settling view could be taken. Now the words, the spoken value, the button and the Details note all come from one fact,
ResultCardActions.lineOffersViewviaResultPresentation.offeredView. Without a photo to take, the line reads "Needs an installer to check", which is what Details already said for such views. Every line that offered "Show me" before still does.UI changes
From
ResultCardUITestson the iPhone 17 Simulator at f0c271c. The before for the photo promise is from #206's run 37112742785. No before exists for the review band, since no fixture had one at 75f6c37. Its before wording is quoted above.A line promises a photo only when it offers one
The review line on the card, default and largest text size
No spot and no nearest spot: no floor zones
Verification
Pull-request run 37126344528 on 55fb4b4: 664 package tests in 93 suites passed, and 63 UI tests ran with 1 skipped and 0 failures, all seven
ResultCardUITestsincluded.55fb4b4 (review, Codex): the review line's wording is inclusive, "anything 15 ft or more" / "anything 4 ft or less". A confident pass needs measured + error < review_threshold_ft, so a value exactly on the line already needs review.
reviewBandApplieswas already inclusive.Hosted full run 37117545464 on f0c271c: 664 package tests in 93 suites passed, and 64 UI tests ran with 1 skipped and 0 failures, including all seven
ResultCardUITestsandInstallerReviewUITests.Package tests:
ResultMarkLayoutTests(no size gives no zones; a sized run matches the old span and depth),ReviewBandTests(decoding, round trip, schema, and 18 cases ofreviewBandApplies, including exact boundaries and an at-least check),ResultCardActionsTests(every combination forlineOffersView), andResultReadingTestsover the three new fixtures.The new fixtures
no-spot-no-nearest.json,review-band.jsonandview-not-offered.jsonvalidate against result.schema.json in the package suite.ResultCardUITestsruns at default size and the largest accessibility size. It checks the labels, the spoken values, that no handoff words appear, and that a line without a photo isn't a button.Review: two reviews found three issues.
Local runs: none. The shared Mac's QA queue is on a safety hold, so the hosted runs are the only execution.
Risk
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit