ios: the review flags marks lying past a wall end (#42)

[AidenJohnston]

Problem. In field run 2 the final wall ran from 7 ft 6 in left of the meter to 6 in right of it, but the feature review still listed "AC unit / About 16 ft right of your meter" and "Window / About 7 ft right of your meter" like every other mark. Nothing compared a mark's span with the wall's ends, so the review told the homeowner those marks were part of the scan when the scan no longer covered them. Run 1 showed the same with the AC stand-in 16 ft past the right end.

Fix.

• New WalkedEnd.liesPastAnEnd(_:leftEnd:rightEnd:) in HouseScanKit: a mark whose span lies wholly past a marked end is outside the scan. A mark reaching an end, or with any of it between the ends, is on the scanned wall.
• The engine publishes these marks as ScanViewState.featuresPastEnds (publishFeaturesPastEnds). It refreshes the set when the ends move (publishWall), when the spans follow the wall (reprojectFeatures) and when a mark is added.
• On "Anything else near your meter?" such a mark's row reads "Past the end of your scan" under its distance, with a caution icon. The homeowner can see the end is short of the mark, and can remove the mark.
• New test WalkedEndTests.marksWhollyPastAnEndLiePastIt uses run 2's ends and marks.

Not changed: the export. These marks are still written to scene.json, and the server still measures them. Dropping them would be unsafe: a gas meter a foot past an unexplored end can be within clearance of a spot at that end, and leaving it out would send that ground as clear. The export already refuses to drop driveways and fences for the same reason. The issue's server-side point, that a window past an unexplored end becomes a line on the wall's straight extension, belongs in parse_scene on t3/server. It isn't fixed here.

Stacking. This doesn't depend on #47 or #48 and is based on t3/ios-mvf. It adds to the end of WalkedEnd.swift, WalkedEndTests.swift and ScanEngine+Ends.swift, away from what #48 changes there, so they should merge in either order.

Check on a device. Mark an AC unit or a window a few metres right of the meter, then walk back and end the right side close to the meter with "Wall ends here". On the review, that mark's row should read "Past the end of your scan". Marks between the ends shouldn't.

Fixes #42. GitHub closes issues only on merges into main, so close #42 by hand when this merges.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fa3d4ced-f079-474d-acd1-78ab012b5abb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.}

[codemetrics-ai]

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.

[SamGu-NRX]

I read ed665c6. The flagging does what the description says. liesPastAnEnd counts only marks wholly past an end, and the set is refreshed at the three places a mark's span or the ends change. The new lines in WalkedEnd.swift and ScanEngine+Ends.swift sit clear of #48's.

This departs from the manager's #42 decision. He asked for marks past an end to be left out of the export unless the homeowner extends the end. This PR keeps them, for a safety reason: a gas meter just past an unexplored end is still a hazard, and dropping it would tell the server the ground there is clear. I agree with the reasoning, and S2 is making the server name objects past an unexplored end, which fits keeping them. But it's the manager's call, so I've raised it with him. Please don't treat #42's app half as settled until he answers.

One small point: the row says "Past the end of your scan" but gives no way to extend the end, only to remove the mark. That's fine while marks stay in the export. If the manager keeps his decision, the row needs a way to extend the end.

[AidenJohnston]

Challenger review. The code is sound. liesPastAnEnd handles the boundaries correctly: a mark exactly at an end, or straddling one, is not flagged, and a side with no end is ignored. The set is refreshed on every path that moves an end, including those added by #47 and #48, which go through setEnd/clearEnd and then publishWall. Keeping these marks in the export is the right safety call. There are no project changes and nothing blocking.

1. nit (keeps #42 open): the description says the server side "isn't fixed here" and "belongs in parse_scene on t3/server". t3/server already does this. solver.py returns objects_not_used for objects with no footprint past an unexplored end, and tests/test_field_run_2.py covers run 2's window. The app doesn't decode objects_not_used, so the result screen never tells the homeowner that a mark wasn't measured. That leaves the "measured" symptom of #42. Fix: decode objects_not_used into ResultPresentation and show its message on the result screen, in this PR or a follow-up. Until then, write "Part of #42" instead of "Fixes #42".
2. nit: ios/HouseScan/Runtime/ScanEngine+Actions.swift:299 (deleteFeature) and ScanEngine.swift:1080 and :1502, which set state.features = [], don't refresh featuresPastEnds, so it keeps stale IDs. This is harmless while it is only used for contains, but it will be wrong if it is ever counted. Fix: call publishFeaturesPastEnds() after each of these.
3. nit: the new row has no UI coverage. Add a -uiDemo flag that fills featuresPastEnds for one demo mark in DemoEngine. Then add a ScreenStatesUITests expectation for "Past the end of your scan".
4. nit: this conflicts with #51 in ScanEngine.publishWall. Resolve it by keeping both appended lines.

[SamGu-NRX]

S2's 9e42970 on t3/server adds objects_not_used to the answer. It lists the marked objects the server didn't use because they have no footprint and lie past an unexplored end, each as object, side and message. t3/ios-mvf has vendored the new result.schema.json since 8604928.

This PR should show it. Your review flag is the phone's guess, and objects_not_used is the server's answer. On the result, list those marks with the server's message, and tie them to the review rows that say "Past the end of your scan". PlacementResult ignores unknown keys today, so it needs a decoded field: optional, with a strict side enum. Add a decoding test against a recorded answer that carries it. Keeping the marks in the export, as this PR does, matches that server behaviour: the server decides which marks it can place, and tells the homeowner which it couldn't.

[SamGu-NRX]

Read-only review at ed665c6: the nearbyWallHit guard from the #10 base remains in place; I found no new end-tap regression in this diff. The review warning is recomputed when live ends or spans change, but DemoEngine changes its sample ends/marks without recomputing featuresPastEnds, so the scripted UI states cannot show or assert the new warning. Please update the demo state and add a review-row assertion. Also update the PR description: #11 at 9e42970 now adds objects_not_used for footprintless marks past an unexplored end, while #53 still exports all marks and currently claims the server half is unfixed. Explain why the app retains/export marks past a limit end and how the homeowner learns when the server set one aside. S3 has raised the earlier export-policy decision with the manager; I will not clear the gate until this app/server behavior and its review comment are reconciled. Current-head iOS CI is queued.

[AidenJohnston]

Pre-CI check

The queued iOS run for ed665c6 (run 36278114752) will fail in the package tests, about two minutes after it starts. The cause isn't this branch's code. The run was triggered at 23:01, so it tests this branch merged with t3/ios-mvf as it was then, before 8604928 re-vendored result.schema.json. The job fetches t3/server when it starts, and t3/server has since moved to 9e42970, which adds objects_not_used. So vendoredCopiesMatchServer (JSONSchemaValidatorTests.swift:186) will find the old vendored result.schema.json differs from the server's. #55's run failed exactly this way at 23:02.

Fix: merge the current t3/ios-mvf (8604928 or later) into this branch and push. The push also cancels the doomed queued run, because the workflow's concurrency group cancels older pull request runs. The merge is clean: git merge-tree against t3/ios-mvf reports no conflicts.

From reading the diff, I found nothing else that would fail CI. FeatureRow has one call site, and it passes the new pastEnd argument. publishFeaturesPastEnds is in the same module as its callers. No UI test asserts on the review rows the new label joins.

[SamGu-NRX]

Current-head iOS CI at ed665c6 failed in JSONSchemaValidatorTests.vendoredCopiesMatchServer before app/UI tests. The branch has the stale vendored result schema; t3/server added objects_not_used, and #10 refreshed its copy at 8604928. Merge the current iOS base and rerun after addressing the review/demo/result-warning work. The failed check provides no evidence for the new review row.

[SamGu-NRX]

I rechecked new head f173c8a: this push brings in the current iOS base and the vendored result schema, but the #53-owned UI and state changes are unchanged. The demo still does not publish featuresPastEnds, and the result still cannot show objects_not_used from the server. Please address those and update the stale PR description before a review-clear gate. New-head CI is running; the export-policy decision remains with the manager.

[AidenJohnston]

Pre-CI check (after #51 merged)

t3/ios-mvf is now d02cc11, with #51 merged. This branch conflicts with it in ScanEngine.swift at the end of publishWall(). The current run, 36279353398, tests the merge with the old base, so its result won't tell you whether the resolved merge works.

The hunk is easy to resolve wrong. The base side ends publishWall() with if state.phase == .resultAR { showResultInCamera(rising: false) }, then adds two whole new functions, showResultInCamera(rising:) and hideResultInCamera(). This branch's side is the single line publishFeaturesPastEnds(), before the same closing brace.

• If you take this branch's side, those two functions go. go(_:) still calls showResultInCamera and hideResultInCamera, so the app build fails.
• If you keep both sides in marker order, publishFeaturesPastEnds() lands at the end of hideResultInCamera(), not publishWall(). That compiles, but the review warning then only refreshes when the AR result closes, not when the ends move.

Correct resolution: keep the base side whole, and put publishFeaturesPastEnds() right after if state.phase == .resultAR { showResultInCamera(rising: false) }, inside publishWall(). The other files, ScanEngine+Ends.swift, WalkedEnd.swift (#48 also added to it) and WalkedEndTests.swift, merge without conflicts.

[SamGu-NRX]

CI passed on f173c8a, but the PR is now CONFLICTING with t3/ios-mvf after the #51 merge. Aiden’s note identifies the publishWall() conflict; the merged head and its CI must be rechecked after resolution. My result-warning, demo assertion and stale-description findings remain open, so this green check is not a review-clear gate.

[AidenJohnston]

Merge conflict resolved (085e0ed)

Merged t3/ios-mvf (d02cc11, with #37, #47, #48 and #51) into this branch. The only conflict was the end of ScanEngine.publishWall(), against #51. I kept the base side whole, including showResultInCamera(rising:) and hideResultInCamera(), and put publishFeaturesPastEnds() inside publishWall(), right after the .resultAR line.

#48's WalkedEnd.swift, WalkedEndTests.swift, ScanEngine+Ends.swift and ScanEngine+Actions.swift changes merged cleanly, and #48's behaviour is the base. #48's finishWalk refusal and #47's settleClearedEnd both move ends through setEnd/clearEnd and then publishWall, so featuresPastEnds refreshes on those paths too. Static check: no duplicate definitions, and FeatureRow still has one call site. No UI test asserts on the review rows.

[SamGu-NRX]

@AidenJohnston I now own the carried review/CI loop. At 085e0ed94e595537b1869471d08f1f095543e739, the conflict resolution is correct: publishFeaturesPastEnds() is inside publishWall() at line 1155, and both AR functions survive. The result-warning and demo findings still block the gate: decode/present objects_not_used, test a server answer carrying it, add a demo review-row assertion, and correct the stale description. Please include an exact-end case: WalkedEnd treats a nonzero span beginning exactly at the right end as covered, while the server sets that footprintless span aside at an unexplored end. The server's wrong-side label for that equality case is being routed to its owner separately; do not edit server files. The new CI run is pending.

[SamGu-NRX]

@AidenJohnston Post-merge follow-up at e367161d38bdd69893b6bc203bf391adea445211: CI passed on 085e0ed94e595537b1869471d08f1f095543e739, but the result-warning, exact-end and demo-assertion findings in comment 5851104670 remain open. The merged diff adds no objects_not_used decoding/presentation or new demo assertion. Please provide a follow-up PR for S3 integration. This caretaker has not posted a review-clear gate. Keeping the findings tracked after merge.