Repository navigation
Conversation
…s one for sharing SavedScanCatalog lists the folders ScanFolderCleanup keeps as completed, newest first, with each bundle's saved time, size and the stamp's practice flag when it is a JSON boolean. A bundle counts only when it was written to the end (PacketArchiveCheck): a quit during the in-place zip write leaves a partial scan.zip under the completed name. SavedScanStaging copies a bundle out of the scan folder for the share sheet, so a cleanup can't delete it mid-share, and removes only its own copies. Links are never scans. Co-Authored-By: Claude Code <noreply@anthropic.com>
…s reopened (#130) The onboarding's top bar opens Saved scans: the completed scans the cleanup keeps, each with its saved time, size and a Practice tag only when its stamp says so. Share copies the bundle to tmp and hands the copy to the system share sheet, which deletes it when it closes; a scan gone since the list was made gets an alert and drops out. Nothing is sent before Share, and a saved scan is never reopened or continued. The onboarding holds the sheet's state: the top bar swaps layouts at accessibility text sizes, and a sheet owned by the button closed when the text size changed. The empty state scrolls, so its text isn't cut off at large sizes in the half-height sheet. Co-Authored-By: Claude Code <noreply@anthropic.com>
…wall wasn't real Practice mode draws a sample meter, and the walk scans a real wall, so "not a real wall" was false. The share copy is not assumed to be a free clone: it can copy every byte and fail on a full disk (copyFailed). Co-Authored-By: Claude Code <noreply@anthropic.com>
…e, not a system list Hosted run 37051620836's audit failed the half-height sheet. The onboarding showed through its glass behind the intro and footer (contrast), the list's header and footer and the navigation bar's Done didn't fully follow Dynamic Type, and large text ran off the bottom. The sheet is now a scrolling column on the canvas, with the rows on a surface card and the title and Done in the app's fonts, like the onboarding. Co-Authored-By: Claude Code <noreply@anthropic.com>
…e since A share sheet open when the app is killed never reports back, so its copy stayed in tmp until Saved scans was opened again. Launch now removes the copies made before it; a share started right after launch keeps its own. Co-Authored-By: Claude Code <noreply@anthropic.com>
…, in the app's init A window's .task runs again for each window or reappearance, and a later cutoff could cover a copy this run is still sharing. Co-Authored-By: Claude Code <noreply@anthropic.com>
…t scan deleted ScanFolderCleanup counted any scan.zip as a completed scan, so a bundle cut short by a quit during its in-place write held one of the two kept places and an intact older scan was deleted instead. Completed now means PacketArchiveCheck finds a whole packet, still the same file after the check. A partial or unreadable bundle holds no place and its folder is left alone at any age: nothing proves its writer has stopped. Retention count, order, listing at construction and the current-folder guard are unchanged. Co-Authored-By: Claude Code <noreply@anthropic.com>
… not just the file Co-Authored-By: Claude Code <noreply@anthropic.com>
… not complete The file-number and size comparison had no test that swaps the file mid-check, so it could stop catching a retry's rewrite unnoticed. The check is injectable within the module for that test. Co-Authored-By: Claude Code <noreply@anthropic.com>
…es the others A launch-time cutoff depended on the clock: set back between runs, an earlier run's copy looked newer and stayed. Each run now stages under a session folder of its own, and launch, or opening Saved scans, deletes the other sessions' folders, so no copy a share in progress is reading can match. removeAll is gone with its last caller. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ix/cleanup-partial-bundles
…lace holds no kept place A top-level link to a scan folder resolved to that scan's bundle, so the link and its target could take both kept places and an intact scan was deleted. A link, or an entry whose type can't be read, now holds no place and is left alone; a plain file still goes, as before. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ans candidate
…ss a new scan's cleanup Two sheets in one process share a staging session, so opening one never deletes the copy the other is sharing. A copy being shared keeps the bundle's bytes after a new scan's cleanup deletes its source folder. Co-Authored-By: Claude Code <noreply@anthropic.com>
…d file after the app closes With the scan packaged, the note now says that closing the app ends the retry but not the sharing: Saved scans on the first screen offers the same bundle. It promises no reopening or retry after close. The note without a bundle is unchanged. The test assertion moves with it. Written with the backend owner's handoff of these lines. Co-Authored-By: Claude Code <noreply@anthropic.com>
…n listed The deletion runs after the listing, off the main actor. A folder listed as never packaged could gain a bundle, partial or whole, before it ran, and was deleted anyway; so was a kept-out bundle a retry had rewritten. run() now reads each obsolete entry again and leaves any that changed, by type, bundle state, save time or file number, for the next cleanup. PacketArchiveCheck's docs now say it detects a write that reached the end in the packet's layout, not that a file is a capture packet. Co-Authored-By: Claude Code <noreply@anthropic.com>
… bundle does Co-Authored-By: Claude Code <noreply@anthropic.com>
SamGu-NRX
added
the
full-ui
|
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. |
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. |
|
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 (19)
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 change adds a saved-scan catalog and sharing interface. It verifies scan archives before listing or cleanup, stages verified copies for sharing, and removes staged copies after sharing or during later-session cleanup. ChangesSaved scans
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor User
participant OnboardingScreen
participant SavedScansSheet
participant SavedScanCatalog
participant SavedScanStaging
participant ActivitySheet
participant UIActivityViewController
User->>OnboardingScreen: Open Saved scans
OnboardingScreen->>SavedScansSheet: Present sheet
SavedScansSheet->>SavedScanCatalog: Load saved scans
SavedScanCatalog-->>SavedScansSheet: Return scan list
User->>SavedScansSheet: Select Share
SavedScansSheet->>SavedScanStaging: Stage verified archive copy
SavedScanStaging-->>SavedScansSheet: Return staged file URL
SavedScansSheet->>ActivitySheet: Present staged file
ActivitySheet->>UIActivityViewController: Present share interface
UIActivityViewController-->>ActivitySheet: Complete or cancel activity
ActivitySheet-->>SavedScansSheet: Run completion closure
SavedScansSheet->>SavedScanStaging: Remove staged copy
Merge Risk: ⚪ Minimal · up to No actionable issue is established for the Saved scans change. Complete the normal combined-tree and device checks before shipping. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Sharing remains user-directed, and temporary copies have cleanup paths. The remaining design concern is that interrupted scans can retain private capture data without an app-enforced expiry, outside the two-completed-scan limit. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 77 functions across 18 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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let files = FileManager.default | ||
| return obsolete.compactMap { url in | ||
| guard files.fileExists(atPath: url.path) else { return nil } | ||
| guard let listed = listedAs[url], Self.entry(at: url) == listed else { return nil } |
There was a problem hiding this comment.
When a scan is started over, its packet write can continue while the new store runs cleanup. If that write starts after this check but before removeItem, cleanup deletes the folder the writer is using. The write can fail and the scan can be lost. The recheck narrows the window, but deletion and writing still need coordination.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift
Line: 140
Comment:
**Cleanup can delete an active write**
When a scan is started over, its packet write can continue while the new store runs cleanup. If that write starts after this check but before `removeItem`, cleanup deletes the folder the writer is using. The write can fail and the scan can be lost. The recheck narrows the window, but deletion and writing still need coordination.
---
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.
Yes. That window is real, and this change doesn't close it. run() reads each listed entry again right before deleting it, so a write that started any time before that read keeps the folder (lines 132–135). A write whose first byte lands between that read and removeItem is still lost. Nothing on the reading side can see it: removing the folder still races with a writer that hasn't started yet.
Closing it needs the writer to cooperate. Either the packet code registers the folder it's writing to, so the cleanup can skip it, or it writes scan.zip under another name and renames it into place. Both are producer changes, and this candidate deliberately touches no producer code. The PR's Risk section and the comments at lines 107–109 and 132–135 state this limit.
There was a problem hiding this comment.
Agreed. The second read protects against changes already observable when cleanup rechecks the folder, but it cannot detect a writer that starts in the final read-to-removeItem interval. Closing that last race requires producer-side coordination—such as writer registration—or atomic publication by writing elsewhere and renaming, both outside this candidate’s scope.
With the limitation explicitly documented in the PR, I consider this a known residual risk rather than an additional issue with this change.
| case .unjudged, .folder(.incomplete): | ||
| incomplete.append(name) | ||
| } | ||
| } | ||
| completed.sort { ($0.date, $0.name) > ($1.date, $1.name) } | ||
| let keptNames = completed.prefix(max(0, keepCompleted)).map(\.name) | ||
| let keep = Set(keptNames + incomplete) | ||
| keptCompleted = keptNames.map { root.appending(path: $0) } | ||
| incompleteBundles = incomplete.map { root.appending(path: $0) } | ||
| let obsoleteNames = names.filter { !keep.contains($0) } |
There was a problem hiding this comment.
Every partial or unreadable bundle is kept regardless of age or count. That protects a live write, but interrupted writes left by earlier app runs have no reclamation path. Over time, those abandoned bundles can use the storage needed to finish or share another scan.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift
Line: 60-69
Comment:
**Partial bundles accumulate indefinitely**
Every partial or unreadable bundle is kept regardless of age or count. That protects a live write, but interrupted writes left by earlier app runs have no reclamation path. Over time, those abandoned bundles can use the storage needed to finish or share another scan.
---
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.
This is the trade-off we chose on purpose, and the type's doc comment (lines 17–23) and the PR's Risk section both state it. A partial bundle can't be told apart from one whose writer is paused: Start over can leave the previous scan's write running, and a suspended phone can pause a write for any length of time. So deleting by age or count could take a folder out from under a live write and lose that scan. An age-based grace period was considered and rejected, because age doesn't prove the writer finished. Reclaiming abandoned partials safely needs proof that no writer holds them, and that belongs in a separate change together with the producer.
There was a problem hiding this comment.
That makes sense. Given the deliberate safety trade-off and the documented limitation, I agree this is not an actionable issue for this change. Safe reclamation should be handled separately alongside producer-side writer-lifetime or atomicity guarantees.
|
@coderabbitai review |
✅ Action performedReview finished.
|
… packaged #203 wrote its notes before #205's Saved scans existed, so a packaged scan's note still said House Scan can't reopen it after the app closes, with no mention that Saved scans on the first screen can share its file then. Now the first note says so and that it can't try again from there, and the repeated note adds that Saved scans can share it later. Without a packaged scan the notes are unchanged, as is the failed-upload note #205 already aligned. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A reviewable candidate, not a ship request. It combines #200 (Saved scans, head
95eee1ed) and #202 (cleanup counts only whole bundles, head74640d5b) on #191's head75f6c377, and adds the failed-upload note, a cleanup fix and tests found during integration. #200 and #202 stay open at those heads; nothing here was pushed to their branches.What changed
After closing the app, a homeowner can still share a finished scan. The first screen's top bar has a Saved scans button. It lists the completed scans the phone keeps (the two most recent). Each row shows when its bundle was saved, its size, and a Practice tag only when the scan's
scan-stamp.jsonsays so. Share copies that scan'sscan.zipinto the app's tmp folder and opens the system share sheet. Nothing leaves the phone until the homeowner picks a destination there. A saved scan can't be reopened or continued, and the sheet says so. It shows no result and restores no AR session, request or placement. Starting a new scan works as before.A failed upload points to Saved scans. When the scan is packaged, the failed-upload note now reads "Keep this scan open to try again. If you close the app, you can still share its saved file from Saved scans on the first screen." It promises no retry or reopening after close. The note for a scan without a bundle is unchanged.
UploadingScreen.swiftand its assertion inScreenStatesUITests.checkOfflineRecoverychange together. The backend owner, who is adding an unusable-answer case to the same screen, handed off these lines.Only bundles written to the end count, are listed or are shared.
scan.zipis written in place, so a quit mid-write leaves a partial file under the completed name. The cleanup, the list and Share all usePacketArchiveCheck, which reads at most 64 KiB from each end. A partial, unreadable or linked bundle, or a link in place of a scan folder, takes neither of the two kept places. It isn't listed, and the cleanup doesn't delete it. Share checks the source and the copy again, so a bundle deleted or cut short since listing gives an alert and drops out of the list.The cleanup deletes only what it listed, still as it was.
ScanFolderCleanuplists folders when a new scan's store is made and deletes them later, off the main actor. A folder could gain a bundle in between, or a retry could rewrite one, and it was deleted anyway.run()now reads each listed entry again and leaves any whose type, bundle state, save time or file number changed for the next cleanup. This was added in this candidate, after a review of the combined branch.A share keeps its own copy. Each app run stages copies under its own session folder in tmp. The sheet deletes its copy when it closes, and launch deletes copies earlier runs left. A new scan's cleanup works on
Caches/Scansonly, so it can't remove a copy the share sheet is reading.Why
#130 asks for an app consumer of the completed scans the phone already keeps. Before this, a finished scan survived a relaunch on disk, but only the session that made it could share it.
Producer dependency. The feature reads the producer's existing output and changes no producer code. It relies on these properties of
scan.zipasKeyframeStore.zipPacketwrites it today:manifest.jsonas the first entry at offset 0, matching the central directory's first record;practiceboolean.If the layout changes, the check fails closed: the scan isn't listed or counted, and its folder is kept. If a producer wrote a valid footer over missing content, or a foreign zip starting with
manifest.jsonwere in a scan folder, the check would pass. Its docs now say it detects a write that reached the end, not packet identity. The missing raw-recorder lifetime proof stays an export limitation: Saved scans shares whatever bytes the producer wrote.UI changes
The failed-upload note, before and after. Both come from hosted CI on the Simulator (iPhone 17) in the frozen
-uiDemoOfflinestate.b0b785d7, run 37075085816)02ff81d7, run 37107646593)Saved scans is new, so these captures are after-only. They come from hosted run 37075085816, which tested merge commit
b0b785d7(75f6c377plus #200). Its tree matches this branch's first mergef1767e31, and the sheet hasn't changed since.Verification
Hosted runs check out a merge commit, not the branch head, so each line names the commit that was actually checked out and how its tree compares.
75f6c377: run 37075085816 checked outb0b785d7(95eee1edmerged into75f6c377), whose tree is identical to this branch'sf1767e31. HouseScanKit ran 667 tests with warnings as errors, all passing. The UI tests ran 59, with 1 skipped and none failing, including bothSavedScansUITestsand the full accessibility audit.b1e3983f, whose tree is identical to74640d5b. That tree is on the older0b35f65cbase, not75f6c377. It ran 666 HouseScanKit tests and 48 UI tests, none failing.02ff81d7, which merges041dae0einto75f6c377. Its tree,6ece05d7, is identical to this branch's head, so the run tested exactly this branch. HouseScanKit ran 676 tests with warnings as errors, all passing. The UI tests ran 59, with 1 skipped and none failing, including bothSavedScansUITests, both offline-recovery tests (standard and largest text size) and the full accessibility audit.Not run: a real device, a real capture's bundle, and finishing a transfer through AirDrop, Mail or Files. The UI tests open and close the share sheet without sending anything. The app is iPhone-only with
UIApplicationSupportsMultipleScenesoff, so two windows can't happen today. A HouseScanKit test covers two sheets in one run sharing a staging session.Risk
scan.zipatomically, which is producer code outside this change.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit