Repository navigation
Conversation
…tery and elec_box A box is marked by the two corners where it meets the wall, like an opening. scene.json carries it as an object of the server's type (server/schemas/scene.schema.json objects[].type) with its span and heights and no footprint: the phone doesn't measure how far it stands out, and no depth is assumed for somebody else's unit (#52's review). The local packet 1.1 gains mark kinds of the same names, two points each, in the vendored manifest schema and its recorded checksum. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d the walk The chips gain "Existing battery" and "Box on the wall". Each is marked by its bottom-left and top-right corners where it meets the wall, the back corners, since a tap lands on the wall's plane and a front corner would land elsewhere along it. The export sends them as scene.json's battery and elec_box with no depth, the packet as marks of the same names, and the review row says how far it sticks out isn't measured. Ports #52's final version (821cee3) to alpha. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
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. Comment |
|
@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: 8fa5cc81a8
ℹ️ 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".
| objects.append(.init( | ||
| type: kind.rawValue, wall_id: wallIDAt((span.lowerBound + span.upperBound) / 2), span_ft: spanFeet(span), | ||
| bottom_ft: feet(bottom), top_ft: feet(top), attrs: nil, source: "tap", footprint: nil, | ||
| plus_minus_ft: objectError)) |
There was a problem hiding this comment.
When an existing battery is marked, this export deliberately omits its footprint, so the server treats it as only the stretch of wall line in span_ft. For a deep battery near a candidate's corner, battery_clearance can therefore compute a passing distance to the wall line even though the real front face is closer and may violate the clearance. The review note disappears once the scan is submitted, so the result can present this unsafe placement as clear; preserve the unknown-depth condition as UNSURE/manual review or collect a conservative depth before allowing a passing answer.
Useful? React with 👍 / 👎.
Ports #52's intent to alpha. #52 built this against a base that no longer exists, and all 14 of its files have moved since, so this is a re-implementation of its final version (
821cee30), not a cherry-pick.What changed
A homeowner can now mark a battery already on the wall, or another box such as a disconnect, sub-panel, EV charger or solar box, from the review's "Add something" chips and the walk's "What do you see?" tray. The new chips read Existing battery and Box on the wall. Each takes two taps, the bottom-left and top-right corners where the unit meets the wall, the same way a door or window is marked.
The marks reach the server under the names it already scores:
batteryandelec_box(server/schemas/scene.schema.json:57). The solver collectsbatteryobjects atserver/solver.py:244and holds a new batteryclearances.battery_ftaway from them inbattery_clearance(server/solver.py:923,server/rules.yaml:55, tests inserver/tests/test_existing_battery.py). The rules treatelec_boxas wall equipment (server/rules.yaml:71), and the cable route detours around both (server/rules.yaml:106,:111).scan.zipbehind "Share scan") gains mark kinds with the same two names, two points each. The copied manifest schema and its recorded checksum change with them.The review row for either kind says "How far it sticks out isn't measured", in the same muted line the AC unit row uses for its assumed size. The review heading now lists other batteries and boxes on the wall.
Why
The server could already score an existing battery, but the app had no way to mark one. Alpha's
ScenePointObjectKindandPacketMark.Kindstopped at gas meter and AC unit.The prompts ask for the back corners because a tap lands on the wall's plane. A front corner, standing out from the wall, lands somewhere else along it when tapped from one side. #52's review found this and added a test for it.
No depth is sent, as #52's final version decided. The taps measure where the box meets the wall, not how far it stands out, and #52 dropped its assumed 22 in footprint because that was Base's own battery depth applied to somebody else's unit. Without a footprint the server measures to the box's stretch of wall line (
server/schemas/scene.schema.json:74). That stretch can be closer than the box's front face near a corner, so the server may pass a spot the real box blocks.How it works
SceneFeature.box(kind:span:bottom:top:)withSceneBoxKind(battery,elec_box) in SceneExport.swift writes asource: "tap"object with span, heights and nofootprint.PacketMark.boxin PacketMarks.swift writes the two wall-face corners in the meter frame.PacketMark.from(.box)derives the mark from the same scene feature.FeatureKind.batteryand.elecBoxreuse the door and window path for projecting taps, clamping heights and drawing the camera bracket.ResultScene3Ddraws them as illustrative boxes, the way it draws the gas meter and AC unit.UI changes
None captured.
ScreenStatesUITestsrenders the Mark screen and gains amarkFeatures-markingBoxstate, but this run didn't boot a simulator, so there are no screenshots. Visible changes: two new chips (8 instead of 6, in the review's two columns and the tray's three), the longer review heading, the new prompts, and the row note.Verification
cd ios/HouseScanKit && ~/.long-run/bin/heavy swift test: 971 tests in 131 suites passed, with the 1 known issue (HeldCaptureReplyHTTPTests.aSecondHeldReplyFailsWithoutLosingTheFirst). This branch adds 2 tests to the alpha suite.BoxMarkTests.aBoxRoundTripsThroughThePacketexports a battery and a box, writes them into a packet as marks and in its scene.json, and decodes both back. The manifest must validate against the manifest schema with kinds[battery, elec_box]and the wall-face corners. scene.json must have objects of typebatteryandelec_boxwith the right span and heights and nofootprint.BoxMarkTests.boxKindsAreTheServersObjectTypeschecks that both names are in the copied scene schema'sobjects[].typeenum and the manifest's mark kinds.vendoredCopiesMatchServerholds that schema copy to the server's file.PacketMarkTests.pointCountsPerKindandvendoredManifestSchemaIsTheRecordedRevisionare updated for the new kinds and checksum.xcodebuild -scheme HouseScan -destination 'generic/platform=iOS Simulator' build CODE_SIGNING_ALLOWED=NO:BUILD SUCCEEDED.ScreenStatesUITests.testReviewMarksAnExistingBatteryAndABox(ported from Mark an existing battery or another box on the wall (#27) #52: mark both, check prompts and the row note) and the lost-place chip list were edited but not compiled or run, because thebuildaction doesn't build the UI test target and no simulator was booted.Risk
ResultReading, B17). Nothing on the result screen yet names the unmeasured box; Mark an existing battery or another box on the wall (#27) #52 added that line and routed every such answer to an installer. See the questions below.Questions for Sam
ResultReading.answer(unmeasuredMarks:), a result footnote, and an AR title that no longer says the battery could go there)? Alpha already reads a passing answer as a candidate an installer checks, so this PR leaves the result screen unchanged.Checklist
markFeatures-markingBoxscreen state will produce one on the next UI test run.🤖 Generated with Claude Code