Repository navigation
Conversation
…spot check A homeowner who can't see or reach the spot's area had to guess between "It's clear" and "Something's there". The spot check now offers a third answer, the same size as the other two, with and without a photo. The answer is typed end to end (ScanActions.answerSpotCheck(_:), SpotCheckAnswer, SpotConfirmationAnswer.cannotCheck). It means not observed: the area's wall, ground and walked-path claims are withdrawn and the scan is sent again, as for "Something's there", and the guidance log closes the check as skipped. When the returned answer names the same area, the earlier answer settles it as given, so the result says the area wasn't checked and needs a person, and nobody is asked again. A moved or larger area is asked about as before. Tests: HouseScanKit cases for the three meanings, recording, same-area settlement and changed areas; demo UI tests for reachability at default and AX5 sizes, the acknowledgment and the result notice; and an engine test on the synthetic replay (-autopilotCannotCheck) that checks the withdrawn stretch in both scenes and that the notice survives a ground refine without a second question. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (17)
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 scan flow adds “I can’t check this area” as a spot-check answer. It withdraws coverage claims for the selected area, records it as unchecked, and can submit another upload. The result screen reports unchecked areas, including areas outside the result’s named spot. ChangesUnchecked spot-check flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor Homeowner
participant ScanEngine
participant CoverageMap
participant SampleResultClient
Homeowner->>ScanEngine: Submit cannotCheck answer
ScanEngine->>CoverageMap: Withdraw claims over the chosen area
ScanEngine->>SampleResultClient: Submit follow-up upload
SampleResultClient-->>ScanEngine: Return configured or bundled sample result
ScanEngine-->>Homeowner: Show result with unchecked-area state
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The new answer and unchecked-area notices are covered by the reported tests. The real-server moved-spot case remains untested, but no specific failure is established; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new answer avoids treating an uninspected area as clear and preserves warnings when the result changes. No new security exposure was identified in the reviewed flow, but recovery after app termination and compatibility with the receiving service were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 56 functions across 16 files. (1 skipped: 1 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. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The reachability and result-notice tests now use #204's whole-frame scrolling instead of isHittable, so they scroll to the condition they assert. A notice taller than the window is read top edge first, then bottom edge, as a homeowner would scroll through it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
After "I can't check this area" the area's claims stay withdrawn for the scan, but the result only said so when the answer sent back named the same spot. An answer with no spot, or a spot elsewhere, dropped the context (Greptile on #206). The scan's confirmation records are the one source: SpotConfirmations.leftAreaUnchecked is true once any check was answered "I can't check this area". ScanEngine republishes it to ScanViewState.spotAreaUnchecked whenever spotConfirm changes, so recording sets it and the existing resets (Start over, a new wall) clear it. The result keeps the spot's own notice where it applies and otherwise shows a wall-wide one: "You couldn't check an area along this wall, ...", which names no spot and no obstruction. Tests: a kit case for leftAreaUnchecked through later answers and a new record; demo result without a spot at default and AX5; and two engine runs that answer the re-upload without a spot (-sampleResultAfterSpotAnswer, debug builds) after "I can't check this area" and after "Something's there". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
The engine set SampleResultClient.answerFile when a spot answer sent the scan again and never cleared it, so after Start over or a new wall the next scan's first upload could get the old file (review of 7071b95). The engine now derives it from the spot records in spotConfirm's didSet, beside spotAreaUnchecked: set while this scan has an answer other than "It's clear", nil once the records reset. Debug-only test hook; release builds never parse the option. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
With "I can't check this area" answered about two different spots, the second result showed only the notice about "this spot" and hid the wall-wide one, so the first area went unmentioned (Greptile on #206). The records now answer the question the result asks: SpotConfirmations.leftUnchecked(besides:) is true when an area other than the shown spot's was answered "I can't check this area" (any such area with no spot shown). The engine keeps the shown spot's area in SpotConfirmState.shownArea and republishes the answer as ScanViewState.uncheckedAreaElsewhere from spotConfirm's didSet. The result shows the wall-wide notice whenever that is true, worded "You also couldn't check another area ..." beside a notice about the spot. Tests: kit cases for areas held by, and apart from, the shown spot; demo result with both notices; an engine run where the answer after "I can't check this area" names another spot, answered clear. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
…lier answer leftUnchecked(besides:) left out every "I can't check this area" answer whose area held the shown spot. After that answer, "It's clear" about a larger area round the same footprint, and the original area shown again, the clear answer settled it: the result had no notice about the spot, and the earlier answer was left out by geometry alone, so nothing said the area was unchecked though its claims stay withdrawn (review of 6cd1248). An answer about the shown spot is now left out only when the answer that settles the spot is "I can't check this area", which is when the result's own notice says it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: 6cd1248d1d
ℹ️ 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".
| records.contains { record in | ||
| record.answer == .cannotCheck && !(shown.map { record.area.holds($0) } ?? false) |
There was a problem hiding this comment.
When area A is answered cannotCheck, a later, larger area B is answered clear, and a subsequent upload returns A, settling(A) selects the newer clear answer from B, but this predicate suppresses the original unchecked record merely because A contains the displayed spot. The result then has a clear spotCheck and uncheckedAreaElsewhere == false, so it shows no warning even though A's coverage claims remain withdrawn; exclude a record only when that specific cannotCheck record is the answer being shown.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2ef03f. The record is now left out only when it is what the shown spot's own notice stands for, which means the answer that settles the shown area is "I can't check this area" (leftUnchecked(besides:)). In your sequence (A cannotCheck, then a larger B clear, then A shown again), B's clear answer settles A, so A's earlier record still counts and the result keeps the warning. SpotConfirmationTests.aLaterClearAboutALargerAreaLeavesTheEarlierUncheckedOneToSay pins it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2ef03f797
ℹ️ 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".
| public func leftUnchecked(besides shown: SpotArea?) -> Bool { | ||
| let spokenFor = shown.flatMap { area in settling(area)?.answer == .cannotCheck ? area : nil } | ||
| return records.contains { record in | ||
| record.answer == .cannotCheck && !(spokenFor.map { record.area.holds($0) } ?? false) |
There was a problem hiding this comment.
When area A is answered cannotCheck, the server returns the same spot with a larger clearance area B, and B is also answered cannotCheck, the result's spot notice already describes B (and therefore A). However, A does not hold B, so this predicate also sets uncheckedAreaElsewhere and the UI incorrectly claims that the homeowner could not check “another area along this wall.” Treat earlier unchecked records contained by the currently shown unchecked area as spoken for as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. When A and then a larger area B around the same footprint are both answered "I can't check this area", the result shows B's own notice and also an extra "another area" notice for A, which B already covers. The claims over both stay withdrawn, the spot's own notice is right, and nothing passes that shouldn't. The only problem is a redundant line of wording. We're keeping e2ef03f as it is for this batch and listing this as a known limit in the description. I'm leaving the thread open for a person to resolve.
…branch: let the homeowner answer "I can't check this area" on the spot check ResultClient.swift conflicted where #203 replaced UploadError.missingSample with MissingSampleResult and #206 added the answerFile early return; both are kept: the answer file first, then the bundled sample with #203's error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… 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>
This branch has not been deployed
1 participant
Stacked on #204 (
fix/spot-fallback-20261003). This PR adds one commit on top of it.What changed
Before the result, the spot check asks whether anything stands in the battery's area. Until now it offered only "It's clear" and "Something's there", so a homeowner who couldn't see or reach the area had to guess. The spot check now offers a third answer, "I can't check this area", with and without a photo. It has the same size and style as the other two, so it doesn't read as a lesser option.
The answer means the area wasn't observed. It is neither an obstruction nor a clear answer:
CoverageMap.withdrawClaims), so scene.json reports the area unseen, and the scan is sent again. The guidance log closes the check asskipped, the packet's existing generic outcome. The packet and scene schemas are unchanged.SpotConfirmations.settling). The result then says: "You couldn't check the area around this spot, so your scan leaves it out as not checked. Someone would need to check it in person." The homeowner isn't asked the same question again. A spot that moved, or an area that grew, is asked about as before.SpotConfirmations.leftUnchecked(besides:)), and the engine republishes it asScanViewState.uncheckedAreaElsewherewhenever they change, so Start over and a new wall clear it with the records.The answer is typed throughout.
ScanActions.answerSpotCheck(clear:)becomesanswerSpotCheck(_: SpotCheckAnswer), and bothSpotCheckAnswerand HouseScanKit'sSpotConfirmationAnswergaincannotCheck. The kit owns what each answer does.keepsClaimsis true only for clear, andguidanceOutcomeismetonly for clear. The engine reads both instead of comparing a Bool. "It's clear" and "Something's there" behave as before.Why
A forced guess corrupts the one input that backs the scan's unobserved claims. A wrong "It's clear" lets those claims stand, and a wrong "Something's there" reports an obstruction nobody saw. An explicit answer for "I couldn't look" keeps the record honest.
This changes the answers, not the area the question covers. The outline and the words still describe the same area as on #204.
UI changes
New states, with no before captures (the third button didn't exist):
spotConfirm-answers-photo,-noPhoto,-photo-AX5and-noPhoto-AX5.spotConfirm-cannotCheck-photoand-noPhoto.result-spotNotCheckedand-AX5.result-noSpot-scanNotCheckedand-AX5(an answer without a spot), andresult-spotAndScanNotChecked(beside the spot's own notice).engine-spotConfirm,engine-result-spotNotChecked,engine-result-spotNotChecked-afterRefine,engine-result-noSpot-scanNotCheckedandengine-result-otherSpot-scanNotChecked.The captures come from the hosted iOS run's
ui-test-resultsartifact. No motion changed. The answer card uses the transition it already had.Verification
SpotConfirmationTestscovers four new cases. Each answer keeps its own meaning. "I can't check this area" is recorded as given and settles the same or a smaller area as itself. It doesn't settle a moved, wider, deeper or shifted area. It and "Something's there" replace each other as the latest answer.SpotConfirmUITestshas three new tests on the demo. All three answers can be tapped, are at least 44 pt tall and stay on screen, with and without a photo, at the default size and at AX5, where the test scrolls to them. The acknowledgment's exact words are checked. The result notice is checked word for word at both sizes, together with the accessibility audit. "Something's there" keeps its own notice, and "It's clear" shows none.SpotCannotCheckUITestsruns the real engine on the synthetic wall with the sample answer (-autopilotCannotCheck). It audits the engine's spot check with three answers. It then checks that both the uploaded and the exported scene.json claim no wall, ground or facing span over the withdrawn stretch, and that the result shows the not-checked notice and not the obstruction notice. After a ground refine takes the answer down (-injectGroundRise), the result returns with the same notice, and the spot check never reappears.-sampleResultAfterSpotAnswer) answers the uploads after the first spot answer from a fixture: after "I can't check this area", an answer without a spot (reject-nearest.json) and one naming another spot (spot-left.json, answered clear) both show the wall-wide notice, and after "Something's there" an answer without a spot shows none.testFullFlowFromRealReplay, which needs a local recording) and 0 failures. That includes all 9SpotConfirmUITests, all 4SpotCannotCheckUITestsandFullFlowUITests.testSomethingThereChecksTheWallAgain, the unchanged "Something's there" path. The earlier head 892acdc passed run 37112742785.Risk
SpotConfirmation.swift). The claims stay withdrawn and nothing unsafe follows; only the wording repeats. Left as a known limit.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit