fix(macos): notify when the scan finishes, and make "Stop after current" respond - #377
Merged
Conversation
…nt" respond Two things found hand-testing the release candidate. The scan never posted a completion notice. It is the step people walk away from — it can run for minutes on a full disk and, unlike the clean, it ends by just sitting there with a number — so it now opts into the same `notifyOnEnd` the real run already used, inheriting the Settings toggle and staying silent for anyone who turned notifications off. It also needs its own `finalDetail`. The notification body defaults to whatever the last streamed HUD line happened to be, and for a task report that is the "=====" separator, so the first notice arrived as a row of equals signs. It now says what was found, or that there was nothing. "Stop after current" looked broken. `cancelUpdateAll()` set a flag that was private and not @published, so SwiftUI could not observe it and the button never changed state — the stop was queued correctly but nothing on screen said so, and the wait for the in-flight item made the click look ignored. The flag is published now, the label becomes "Stopping after current…", and the button disables so it cannot be pressed twice.
📝 WalkthroughWalkthroughThe pull request adds completion notifications to cleanup dry runs. It updates the update-all stop button to show and enforce pending cancellation. It also corrects staging-directory suffix assertions. ChangesCleanup and update controls
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The assertion I added in #367 read "no X remains in the suffix", on the theory that a leftover X meant mkdtemp had not substituted the template. It does not: mkdtemp draws replacements from an alphanumeric set that INCLUDES 'X', so a perfectly good name like BurrowUpdate.YmzfxX tripped it. Roughly a one-in-eight chance per directory, and it passed locally purely by luck before failing on CI. The failure actually worth guarding against is the template surviving whole, so assert that instead — the suffix is not "XXXXXX" — plus the charset. Ran 30 consecutive times with no failures.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macos/Tests/UpdateWorkflowTests.swift`:
- Line 120: Rewrite the comment at the indicated assertion in
UpdateWorkflowTests to explain why the test allows X in the substituted
characters, or why the previous assertion was invalid, instead of describing the
prefix and suffix checks performed below. Keep the comment focused on rationale
and compliant with the macos Swift WHY-comment requirement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0000c8d7-b4dd-4b06-9ebb-9f70c941c276
📒 Files selected for processing (1)
macos/Tests/UpdateWorkflowTests.swift
| // they passed no matter what create() produced. Assert the real mkdtemp | ||
| // shape instead: the fixed prefix, and six substituted template | ||
| // characters with no X left unreplaced. | ||
| // shape instead: the fixed prefix and six substituted characters. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rewrite the comment on Line 120 to explain the failure mode.
The comment restates the prefix and suffix checks implemented immediately below. Keep the explanation for allowing X, but remove this redundant summary or state why the previous assertion was invalid.
As per path instructions, comments in macos/**/*.swift must explain WHY, not what.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macos/Tests/UpdateWorkflowTests.swift` at line 120, Rewrite the comment at
the indicated assertion in UpdateWorkflowTests to explain why the test allows X
in the substituted characters, or why the previous assertion was invalid,
instead of describing the prefix and suffix checks performed below. Keep the
comment focused on rationale and compliant with the macos Swift WHY-comment
requirement.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs found hand-testing the release candidate.
"Stop after current" looked broken
cancelUpdateAll()setcancelUpdateAllAfterCurrent, which wasprivateand not@Published— so SwiftUI could not observe it and the header button never changed. The stop was queued correctly, but nothing on screen said so, and since it only takes effect after the in-flight item finishes, the click looked ignored for however long that took.Now published, the label switches to "Stopping after current…", and the button disables so it can't be pressed twice.
The scan finished silently, then notified with
======The scan is the step people walk away from — minutes on a full disk, ending by just sitting there with a number — but it never opted into a completion notice. The real clean already did, via
notifyOnEnd, so the scan now uses the same flag and inherits the Settings toggle: silent for anyone who turned notifications off.That alone produced a notification whose body was a row of equals signs. The body falls back to the last streamed HUD line, and for a task report that's the
=====separator:So the scan supplies its own
finalDetail— "12.4 GB found across 37 items." or "Nothing to clean."Verification
1053 tests, 0 failures. Both paths exercised by hand on a Developer ID-signed local build with the engine bundled.
Not a bug, for the record
Two other things turned up while testing and were both artifacts of the local build, not defects:
dev.caezium.Burrowat2(allowed) but a DerivedData build at0(denied), so the scan's.fullDiskAccess(adminBypass: true)gate fires on every elevated step.mo1.46.0 — upstream GPL Mole, not the 1.42.0 fork we ship. Copying the real engine into the bundle restored it.Summary by CodeRabbit
New Features
Bug Fixes