test: migrate suite to Swift Testing and raise coverage - #861
Merged
diazdesandi merged 29 commits intoAug 1, 2026
Conversation
github-actions
Bot
added
appearance
Contributor
|
Labels: Warning This PR looks large (243 files, 34862 lines changed). Consider splitting if possible (see CONTRIBUTING: aim ≤20 files / ≤500 LOC). cc @diazdesandi |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (243)
📝 WalkthroughWalkthroughThe PR centralizes persistence through ChangesThaw modernization and coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Raw `xccov` reports ~22% for this project because it counts every SwiftUI view and WindowServer wrapper. SonarCloud reports 65.3% because sonar-project.properties excludes them. Without applying those exclusions locally there is no way to tell whether a change moved the number that the quality gate actually reports. This reads an .xcresult with xccov, applies sonar.sources and sonar.coverage.exclusions, and prints total coverage plus a per-file breakdown sorted by uncovered lines. Files under Shared/ compile into both the app and the XPC service, so xccov reports them once per target; the best result per path is kept so they are not counted twice. Baseline on this commit: 63.24% (11279 measurable, 4146 uncovered), 1354 tests passing (948 XCTest + 406 Swift Testing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
ThawTests was split across two frameworks: 79 files on XCTest and 32 already on Swift Testing, with no plan to finish. This converts the remainder so the suite speaks one dialect. Pure translation. No test was added, removed, merged, or reordered, and no assertion was strengthened. The mapping preserves semantics rather than collapsing everything into #expect: XCTUnwrap -> try #require (keeps stop-on-failure) XCTFail -> Issue.record XCTAssertThrowsError -> #expect(throws:) XCTAssertNoThrow -> bare `try` in a throws test XCTAssertEqual(accuracy:) -> #expect(abs(a - b) < e) XCTestExpectation -> await confirmation Multi-class files became one outer @suite with nested @suites. setUp/tearDown became scoped `withX { }` helpers, or init/deinit where state spans the suite. Two things needed care. XCTest re-exports Foundation and Swift Testing does not, so files using UUID/Date/Data/JSONEncoder needed an explicit import. And Swift Testing runs tests in parallel within the process while this suite has only ever run serially (parallelizable = "NO" in Thaw.xcscheme), so every suite touching process-wide state carries .serialized for now. That is deliberately conservative and gets unwound once Defaults is injectable. PermissionTests was the one real trap: `onChange` fires on every poll tick, so a naive confirmation translation fails on the second call and continuation.resume() traps. It latches the first grant. Verified: - 1354 tests pass, 0 failures -- identical to the pre-migration count (948 XCTest methods + 406 Swift Testing tests) - coverage byte-for-byte unchanged at 63.24% (7133 covered, 4146 uncovered), which is what a pure translation should produce - swiftlint --strict clean - swiftformat violations 22 -> 21; none introduced Verified on Xcode 27.0 / Swift 6.4 locally. CI pins Xcode 26.5, which is not installed here, so this needs a CI run to confirm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
…tore
Every accessor on `Defaults` hardwired `UserDefaults.standard`, so any code
path that persists a setting was untestable without writing into the real
com.stonerl.Thaw domain of whoever ran the suite. That is why ProfileManager
sits at 39% and DisplaySettingsManager at 26%: the reachable-but-destructive
paths simply were not exercised.
Adds `Defaults.store`, defaulting to `.standard` and never reassigned in
production, plus a `withScratchDefaults { }` test helper that points the
facade at a throwaway suite and deletes it afterward.
This supersedes the per-key snapshot/restore in GeneralSettingsTests and
AdvancedSettingsTests. That approach could only protect keys it knew about,
and its own doc comment flags it as unsound once suites run concurrently.
`Defaults.store` is process-wide mutable state, so suites using the helper
must be `.serialized`. That is documented on the helper.
1354 tests pass, unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
…ries Follow-on to 6142798, which only covered the `Defaults` facade itself. Twenty-five call sites bypassed the facade and used `UserDefaults.standard` directly, so the seam had holes exactly where it mattered. The worst is MenuBarItemManager persisting `savedSectionOrder`: a test touching it rewrites the real menu bar arrangement of whoever runs the suite. All 25 now route through `Defaults.store`, which is `.standard` in production, so behaviour is unchanged outside tests. Also widens two boundaries in DisplaySettingsManager: - `handleExternalPerDisplaySettingsChange` private -> internal, so the URI notification path can be driven with a hand-built Notification instead of `performSetup(with:)`, which needs a live AppState and installs a one-second debounced observer. That path is ~250 uncovered lines. - `parseScope(from:)` private -> internal static. It reads nothing but its argument. `withScratchDefaults` now takes a process-wide lock. `Defaults.store` is a single global and `.serialized` only orders tests within a suite, so without the lock two suites could install scratch stores concurrently and read each other's. The async overload deliberately does not lock -- holding an unfair lock across a suspension point risks deadlock -- and says so. 1354 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
Adds 168 tests across ten new suites, taking measured coverage from 62.89% to 66.67% (7093 -> 7520 covered lines). Largest movement is DisplaySettingsManager, 25.4% -> 50.8%. Its Settings-URI notification path is now driven with a hand-built Notification and a synthetic display UUID seeded into `configurations`, which reaches every per-display setter without a second monitor attached and without `performSetup(with:)`. Also covers ProfileManager's persistence surface (manifest decode failure, directory-creation failure, captureCurrentLayout's Defaults reads, import reconciliation, export skip-on-unloadable, and the reachable throw sites), plus ObjectStorage, Predicates, LocalizedErrorWrapper, MenuBarTintKind, ControlItemImage and Migration, which were at or near zero. Every suite touching Defaults runs through `withScratchDefaults`, so the run no longer writes into the developer's real domain. Four compile fixes were needed on top of the generated tests: the app target sets SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor and ThawTests does not, so ObjectStorage, Predicates and MenuBarTintKind's members are main-actor isolated and their suites needed @mainactor. LocalizedStringKey is Equatable but not Hashable, so the distinctness check compares pairwise. 1522 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
Adds 103 tests across five suites, taking measured coverage from 66.67% to
68.93% (7520 -> 7775 covered lines).
Extensions.swift 45.8% -> 71.2% (482 -> 256 uncovered)
SettingsURIHandler 85.5% -> 86.7%
WindowInfo 33.1% -> 35.1%
Hotkey 21.8% -> 31.9%
Extensions is split across two suites: the non-graphical half (bundle
metadata, MenuBarItem collection helpers, the unfair-lock claim, the Combine
operators) and the CoreGraphics/AppKit half. Every NSScreen member is left
alone -- those answers depend on the number and arrangement of attached
displays, so a test would assert the developer's monitor layout rather than
the code.
Hotkey's most useful addition is that `keyCombinationDidChange` fires exactly
once per assignment and not during init. ProfileManager.rebuildProfileHotkeys
depends on that invariant and nothing verified it.
Two compile fixes were needed on top of the generated tests:
- WindowInfoDecodingTests held its fixture payload in a `static let` of
[String: Any], which strict concurrency rejects as non-Sendable. It is now
computed, which also stops a mutating test from leaking into the next.
- #expect/#require capture their argument into a closure that binds it
immutably, so `removeFirst(matching:)` -- a mutating method -- cannot be
called inside the macro. Those three calls are hoisted to their own line.
1625 tests pass, swiftlint --strict clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
The suites added in the previous two commits carried formatting drift that swiftformat flags: brace placement on multi-line dictionary literals and numeric-literal grouping. Running the project config over the seven offending new files brings ThawTests back to 21/130, the same count the branch started from, so this work adds no new drift. swiftformat is not CI-enforced (only swiftlint --strict is), which is why the pre-existing 21 are left alone -- fixing those is unrelated churn. 1625 tests pass, swiftlint --strict clean, coverage unchanged at 68.93%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
swift:S2961 on MenuBarShapes.swift -- `static let \`default\`` becomes `defaultKind`. Backtick-escaping a keyword makes the declaration read as a language construct at the point of use. One call site. swift:S107 on Profile.swift -- ProfileContent's ten-parameter initializer is deleted in favour of the synthesized memberwise one. The four defaults move onto the properties, and the synthesized initializer takes its parameter order from property order, so all ten call sites are unchanged. ProfileContent is not Codable, so nothing here touches a stored format. The other two findings are deliberately left and should be marked Accepted in SonarCloud rather than fixed: swift:S107 on Profile.swift:188 (AdvancedSettingsSnapshot, 20 parameters). That type has explicit CodingKeys and a hand-written init(from:) but no custom encode(to:), so encoding is synthesized from the stored properties. Grouping them into sub-structs changes the on-disk JSON and breaks every saved profile plus ProfileExportBundle v1. swift:S1075 on HookRunner.swift:29. The path already is customizable: defaultOSAScriptPath is the default for the osascriptPath parameter on run/runIfEnabled, and tests already override it. Sonar is matching the string literal and cannot see the seam. 1625 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
Adds 268 tests across seven suites, taking measured coverage from 68.93% to
72.30% (7775 -> 8148 covered lines).
MenuBarItem 15.3% -> 30.5%
DisplaySettingsManager 50.8% -> 64.7%
SettingsURIHandler 86.7% -> 88.0%
Profile 79.0% -> ~100%
MenuBarItem's whole value surface is reachable through its internal memberwise
init: capability flags, both uniqueIdentifier shapes, per-field equality and
hashing, autoDetectedName's namespace switch and title-casing, and customName
through a scratch defaults store. Tests needing a resolvable sourceApplication
pass the test runner's own pid rather than guessing one, so nothing depends on
which apps happen to be running.
MenuBarSection contributes only its two pure statics, usableInlineWidth and
presentationMode, table-driven over notch and no-notch screens including the
clamping and the <= boundary. The instance half needs a live NSStatusItem.
Two of the generated tests asserted behaviour the code does not have and were
corrected rather than committed red:
- "the converted V1 is written last" claimed V1 overwrites V2 when Ice holds
both appearance keys. importAppearanceSettings is an if/else if, so V2 wins
and V1 is only a fallback. The test now pins that precedence, which is the
behaviour worth protecting: preferring the older key would silently undo an
appearance change made in a later Ice build.
- "a mistyped UseIceBar generates no per-display configuration" was removed.
UserDefaults.bool(forKey:) coerces a stored 1 to true, so the per-display
conversion runs, and that walks NSScreen.screens -- the result depends on
how many displays the machine has attached.
Three test methods in MenuBarSectionGeometryTests take file-private case types
and had to be declared fileprivate to compile.
1791 tests pass, swiftlint --strict clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
Adds 87 tests across six suites, taking measured coverage from 72.30% to 73.59% (8148 -> 8294 covered lines). ProfileManagerDeepTests reaches two catch blocks nothing else could. The existing "occupied directory" test cannot enter ensureDirectoryExists's catch, because a file at the profiles directory path makes fileExists return true and createDirectory is never called; putting a regular file at a *parent* component makes creation genuinely fail. saveManifest's catch is reached by parking a non-empty directory on profiles.json, since an atomic write cannot rename its scratch file over a directory. Each of those tests reloads through a second ProfileManager to prove the swallowed write really failed rather than quietly succeeding. One generated test asserted that deleting from an unreachable directory succeeds. It does not: deleteProfile swallows only CocoaError.fileNoSuchFile, and an ENOTDIR parent surfaces as NSFileWriteUnknownError instead, so the error propagates. The test now pins that, which is the behaviour worth having -- it is what stops a broken profiles directory being reported to the caller as a successful delete. ProfileManager is now effectively exhausted at 35.1%. Roughly 700 of its 742 remaining lines sit behind `private(set) weak var appState`, assigned only in performSetup(with:). Reaching them needs a production seam, not more tests. 1878 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
`MenuBarBackgroundKind.defaultKind == .none` is ambiguous now that the member is no longer backtick-escaped -- `.none` resolves to `Optional.none` as readily as to the case. Spelled out in full. The two averageColor colour-space assertions compared `CGColorSpace.Name?` against `CFString` through a cast chain that inferred an unintended optional. The name is required first, then compared as String. 1889 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
…eams Takes measured coverage from 74.10% to 81.09%, clearing the 80% statement coverage criterion for the OpenSSF Best Practices silver badge. Both files buried pure logic behind system-bound code, so excluding either whole would have contradicted the rule stated at the top of sonar-project.properties -- pure value types and algorithms stay measured even under a UI-sounding path. Instead they are split the way MenuBarItemManager already was when LayoutSolver, PendingLedger, AXIdentityCatalog and ClickReactionVerifier were extracted out of it. MenuBarSectionLayout.swift (measured) takes Name, PresentationMode, notchGap and the three rules that decide how a section is shown. MenuBarSection.swift keeps show/hide/toggle, updateControlItemState and the rehide task and monitor, which need a live ControlItem/NSStatusItem, an AppState and a real NSScreen. MenuBarItem.swift (measured) keeps the value type. MenuBarItem+Enumeration .swift takes the window enumeration, which reaches the window server through Bridging/CGS and resolves owning processes over XPC. No access level was widened. The unchecked initializers, the MenuBarItemTag and Namespace helpers and ResolvedPID moved into the excluded file with the code that calls them, so they stay private. `nonisolated` had to be restored on the two nested enums -- an extension's nonisolated does not reach into nested types. MenuBarItem's explicit memberwise init is now synthesized: it was only written out because the unchecked initializers sat in the struct body and suppressed synthesis. Also excludes three files whose substance genuinely cannot run in CI: AXItemActivator (Accessibility permissions; its two pure helpers were already extracted and stay covered), SettingsDetailStyle (a PrimitiveButtonStyle whose makeBody needs a ButtonStyleConfiguration, which has no public initializer), and OnboardingTour (a SwiftUI View whose navigation lives in @State). The last of those was contributing unstable coverage -- its lines were being counted only when the test host happened to render the onboarding sheet at launch. 1889 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
The suite snapshotted and restored the app's whole persistent domain in
init/deinit. Swift Testing builds a fresh suite instance per test, so that wipe
ran once per test -- eight times -- and each one raced every other suite
reading Defaults concurrently. Its own doc comment said it did this "because
Defaults is hardcoded to .standard", which stopped being true when
Defaults.store landed.
Each test now builds its AppSettings inside withScratchDefaults, which takes a
process-wide lock and never touches the real domain. The suite no longer needs
.serialized and becomes a struct.
Honest note on the flake this was expected to fix: a subagent reported
AutomationSettingsTests failing roughly one run in nine from this interaction.
I could not reproduce it -- six consecutive pre-fix runs came back clean. At
one-in-nine, six runs only had about a even chance of catching it, so this is
not evidence the flake was absent, and the change stands on its own merits
regardless: wiping a shared domain eight times per run while other suites read
it is a hazard whether or not it had been observed failing.
Verified:
- 1889 tests pass, four consecutive runs, zero issues
- `defaults read com.stonerl.Thaw` is byte-identical before and after a full
run, which is the acceptance criterion this work was aiming at
- swiftlint --strict clean
The remaining 37 .serialized suites are deliberately left alone. Defaults.store
is a single process-wide global, so suites that touch it are genuinely mutually
exclusive; removing the trait would reintroduce the class of race this commit
removes. The trait is correct there, not vestigial.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
…s that fit swift-algorithms is already a dependency. Two hand-rolled dedup loops were doing exactly what `uniqued()` does: AdvancedSettings.sanitizedSearchSectionOrder -- parse, drop duplicates keeping first-seen order, then append the missing cases. MenuBarItemTag.canonicalPersistentIdentifiers -- canonicalize, then drop duplicates that differ only in their volatile portion. Both are covered by existing suites (AdvancedSettingsTests, MenuBarItemTagCanonicalizationTests), which is what makes the swap safe. Two further candidates were examined and deliberately left alone, because `uniqued()` would change behaviour rather than express it: HotkeysSettingsPane.makeRows -- `guard let hotkey = ..., seen.insert(id) .inserted` short-circuits left to right, so an item with no hotkey never marks its identifier seen. Hoisting the dedup ahead of the lookup would start consuming identifiers for items that produce no row. MenuBarItemManager.uncheckedCacheItems -- the duplicate branch emits a diagnostic naming the skipped item. `uniqued()` would drop it silently, and that log is what makes a duplicate-window report explicable after the fact. Both files also gained an explicit `import Algorithms`. They compiled without one only because member import visibility is off on the Thaw target, so `uniqued()` was resolving transitively through MenuBarItemManager.swift's import. That upcoming feature is already enabled on MenuBarItemService, so relying on the transitive path here would break the day it is turned on for the app target. 1889 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
MenuBarManager and DisplaySettingsManager already moved their debounced observers to swift-async-algorithms, each with a comment explaining why. ProfileManager was the straggler, still running three Combine `.debounce(for:scheduler: DispatchQueue.main)` chains -- and it was the last thing in the file needing Combine at all, so `import Combine` and the `cancellables` set both go with them. Each observer now follows the shape DisplaySettingsManager established: a NotificationCenter observer feeds an AsyncStream that `.debounce(for:)` coalesces, with the observer owned by its task and removed by a defer when the task ends. That keeps the non-Sendable observer token off the class, so the new deinit only has to cancel three tasks. Each assignment cancels the previous task first, so a repeated performSetup cannot leave an orphaned observer running -- the Combine version had the same protection via `cancellables` being replaced wholesale. Debounce intervals are unchanged: 1.5s for screen parameters, 0.5s for both Focus Filter notifications. Also flips six `var filter` to `let` in FocusFilterIntentTests. ThawFocusFilter's @parameter wrapper is a final class, so its setter is nonmutating and the compiler was warning that the variables are never mutated. Coverage moves 81.09% -> 80.76%. The migration adds lines to performSetup, which needs a live AppState and so stays uncovered; the OpenSSF silver threshold still holds but with less margin than before. 1889 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
Both were surfaced while writing coverage tests, and both were verified in source before being changed. 1. An empty `itemOrder` shadowed `savedSectionOrder`. `captureCurrentLayout` derives `itemOrder` from the item manager's cache, which is empty while the menu bar is still settling, so a capture taken at the wrong moment writes `[:]` rather than `nil`. `resolvedItemOrder` was a plain `itemOrder ?? savedSectionOrder`, and an empty dictionary is still non-nil, so that empty value shadowed a perfectly good `savedSectionOrder` and the next apply saw no layout at all. The fallback now also triggers on empty, which repairs profiles already written that way rather than only preventing new ones. `resolvedItemSectionMap` is built from the same property and is fixed with it. 2. `apply(to:)` discarded the snapshot's own `lastCustomIceIcon`. It assigned `lastCustomIceIcon` before `iceIcon`, and `iceIcon`'s `didSet` mirrors a `.custom` icon into `lastCustomIceIcon`. Every profile carrying a custom icon therefore came back with the two fields equal, losing whichever icon the profile had remembered separately. The assignment now happens after. An existing test pinned that second behaviour as expected -- it was written from observation during the coverage work, before the ordering was recognised as the cause. It now asserts the corrected behaviour, and its doc comment says what changed. Three new tests cover the empty/populated/both-empty cases of the first fix. 1893 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
`handlePerDisplayToggle` accepted any identifier containing a hyphen, while
`handlePerDisplaySetForSpecificDisplay` required it to parse as a UUID and to
name a display that is connected or has a persisted configuration. So
`thaw://toggle?key=useIceBar&display=a-b` posted a notification for a display
that does not exist and returned true, while the equivalent `set` refused it.
DisplaySettingsManager discards such a notification, so the only effect was
telling the caller a toggle had happened when none had.
Toggle now uses the same two guards. That also removes a redundant
`!uuid.isEmpty`, which the enclosing `if let uuid, !uuid.isEmpty` already
guaranteed.
Three tests had pinned the loose behaviour, one of them explicitly ("checks the
identifier's shape, not the display's existence"). Two now seed a persisted
configuration for the display they toggle; the third asserts the refusal and
says in its doc comment what changed and why.
1893 tests pass, swiftlint --strict clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
ASSURANCE_CASE.md still recorded ~44% coverage with `test_statement_coverage80` deferred, dated before this cycle's work. It also named the SonarCloud project `stonerl_Thaw`, which is no longer the key. The criterion it set for itself was "raise coverage (or document an accepted exclusion set with measured coverage of the remainder) before marking Silver coverage Met". Both now hold: 80.8% over 9,960 measured lines from 1,893 tests, with the exclusion set and the rule governing it written down in sonar-project.properties. §6.4 says which code is excluded and why, notes there is still no enforced gate, and records that the residual gap is concentrated in ProfileManager behind a live AppState -- a testability limit rather than untested logic. sonar.projectVersion moves to 2.0.0-rc.2 to match MARKETING_VERSION, which the properties file asks to keep in sync. The marketing version itself is left alone; this work ships in rc-2. Separately, `useIceBar` is removed from `supportedBooleanKeys`. It is a per-display setting resolved through DisplaySettingsManager, not a Defaults-backed global, and it had no entry in `keyMapping`. That only worked because all three handlers test `perDisplayKeys` first and the one loop over the table filters per-display keys out; a new call site reading this table first would have found no mapping and failed silently. `alwaysShowHiddenItems`, the other per-display Boolean, was already absent, which is the shape both should have. `isValidSettingsKey` consults both tables, so the key stays valid. Two tests asserted the old table and now assert the corrected split, including that `useIceBar` remains a valid settings key. 1893 tests pass, swiftlint --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
…raw values
`swiftformat` reported 56 files needing formatting across Thaw, Shared,
MenuBarItemService and ThawTests, all of it pre-existing drift. It is not
CI-enforced -- only swiftlint --strict is -- so the tree had accumulated
inconsistency. All four directories are now clean.
The bulk is cosmetic: wrapIfStatementBodies, indent, modifierOrder, and the
file header on 43 files that were missing the Copyright (Ice) line. The
semantic-adjacent rules that did fire are equivalence-preserving --
hoistAwait, hoistTry, conditionalAssignment, preferKeyPath, redundantVariable.
One rule had to be disabled rather than applied. `redundantRawValues` wanted to
strip the raw values from four cases in Defaults.swift, and those raw values
are the literal UserDefaults key names on disk -- the same values
DefaultsKeyTests pins precisely because they are a stored format. Dropping them
would make each stored key follow its Swift case name, so a later rename would
silently orphan every user's setting with no compile-time complaint. It is now
disabled in .swiftformat with that reasoning recorded, so a future run cannot
reintroduce the hazard.
`redundantSwiftUIGroup` did fire, all of it in TourMockups.swift, and was
allowed: each case is a `Group { if ... }` at the top of a `body`, which the
result builder handles directly.
1893 tests pass, swiftlint --strict clean, coverage unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
Signed-off-by: diazdesandi <diazdesandi@proton.me>
ThawTests had grown to 143 files in one flat directory, which made it hard to tell what a given area was covered by, or where a new test belongs. The layout now mirrors Thaw/ and Shared/: MenuBar/Items 39 MenuBar/Layout 12 MenuBar/Appearance 4 MenuBar/ControlItem 3 MenuBar/Section 2 MenuBar/IceBar 1 Settings/Models 30 Settings/URI 7 Settings/Search 3 Utilities 19 Shared 8 Hotkeys 5 Onboarding 2 Permissions 1 Support 2 Fixtures 2 CrossCutting 5 Support holds the two shared helpers that are not themselves tests (MenuBarTestFixtures, ScratchDefaults). Fixtures is unchanged -- it already held the recorded log data. CrossCutting holds the five CoverageSweep files. Each spans several unrelated types, so no single folder fits, and their names describe how they were produced rather than what they cover. They are worth renaming and probably splitting; grouping them makes that visible rather than hiding it across the tree. Pure moves: 143 renames, zero content changes, and git records every one as a rename so blame survives. ThawTests is a PBXFileSystemSynchronizedRootGroup, so the folders are picked up automatically and project.pbxproj is untouched. sonar.tests = ThawTests is directory-based and follows without change. 1893 tests pass -- the same count as before the move -- swiftlint --strict and swiftformat clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
diazdesandi
force-pushed
the
test/swift-testing-migration-and-coverage
branch
from
July 31, 2026 17:51
201303e to
a7d0213
Compare
github-actions
Bot
added
docs
Include MenuBarItem+Enumeration.swift and MenuBarSectionLayout.swift added by the MenuBar testability split. Signed-off-by: diazdesandi <diazdesandi@proton.me>
…rage Signed-off-by: diazdesandi <diazdesandi@proton.me>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 46
🤖 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 `@scripts/coverage-report.py`:
- Around line 75-90: Replace the hand-rolled option parsing in main with a
parse_args helper using argparse. Define positional xcresult_path as an optional
Path, --top as an integer defaulting to 30, and --json as a boolean stored in
as_json; have main consume the returned Namespace so missing or invalid --top
values produce argparse usage errors and the existing behavior remains intact.
- Around line 46-58: Update is_excluded so patterns ending in "/*.swift" are
handled before the general fnmatch call, enforcing that the matched path has
exactly one filename component beneath the prefix and rejecting nested paths.
Retain fnmatch for other patterns, including recursive Sonar-style patterns,
while preserving the existing boolean exclusion behavior.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift:
- Around line 76-85: Move the `diagLog` declaration out of the documentation
block so the comment directly precedes `getMenuBarItemWindows`. Remove or
relocate the duplicated display/option documentation currently before
`assignStableInstanceIndices(to:using:)`, leaving that method without inaccurate
documentation and preserving the correct comment before `getMenuBarItems`.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 7015-7017: Update the profile-apply flow surrounding
armProfileState to snapshot the prior savedSectionOrder, pinning sets, and
activeProfileLayout and associate the operation with an apply token; when
cancellation is detected in Phase 6 or Phase 7, restore the snapshot only if
that token still owns the state, prevent late-arrival re-sorting from applying
the cancelled profile, and skip scheduling the deferred cache refresh.
In `@Thaw/Settings/Models/FocusFilterIntent.swift`:
- Line 88: Replace the duplicated "FocusFilterRequestedProfileID" literals in
the FocusFilterIntent write path and ProfileManager read path with one shared
key constant, preferably a shared Defaults.Key or static property. Update both
access sites to reference that constant while preserving the existing storage
behavior.
In `@Thaw/Utilities/SettingsURIHandler.swift`:
- Around line 696-713: Extract the UUID-format and display-existence checks into
a private helper returning the validated DisplayIceBarConfiguration or nil after
logging. Update both handlePerDisplayToggle and
handlePerDisplaySetForSpecificDisplay to call this helper and use its returned
configuration, preserving the existing validation behavior and diagnostic
messages.
In `@ThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swift`:
- Around line 29-44: Update the MenuBarItemFailureLedgerTests setup and teardown
to also snapshot, clear, and restore Defaults key .unresponsiveMenuBarItemsBuild
alongside .unresponsiveMenuBarItems. Preserve the existing restoration behavior
for both keys, including removing each key when its original value was absent.
In `@ThawTests/MenuBar/Items/MenuBarItemValueTests.swift`:
- Around line 473-476: Update the assertions in the menu-agent auto-detection
test to verify that autoDetectedName equals the resolved source application name
and is unaffected by the menu title, rather than asserting it lacks “Menu”.
Preserve the non-empty validation and use the existing source-application-name
value established by the test setup.
In `@ThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swift`:
- Around line 110-118: The multipleBlockingFlagsAllBlock test does not cover the
alwaysHiddenSectionResolved and hiddenSectionHasRoom gates. Add separate
assertions or test cases that set each of these fields to false while leaving
the other persistence conditions satisfied, and verify
LayoutSolver.shouldPersistSavedOrder returns false for both.
In `@ThawTests/MenuBar/Layout/LayoutResetErrorTests.swift`:
- Around line 17-33: Remove the three case-existence tests—missingAppStateError,
missingControlItemsError, and alreadyInProgressError—from
LayoutResetErrorTests.swift. Keep the description and recovery-suggestion tests
below unchanged, as they already exercise all enum cases.
In `@ThawTests/MenuBar/Section/MenuBarSectionNameTests.swift`:
- Around line 90-107: Replace the tautological nil checks in localizedVisible,
localizedHidden, and localizedAlwaysHidden with equality assertions comparing
each localized value directly against its expected LocalizedStringKey. Keep the
existing test coverage for all three MenuBarSection.Name cases while validating
the actual key returned by localized.
In `@ThawTests/Permissions/PermissionTests.swift`:
- Around line 19-63: Add the existing .timeLimit(.minutes(1)) trait to
performRequestRestartsPollingAfterChecksStop, matching the async test pattern
used elsewhere. Keep the continuation and polling assertions unchanged so a
missing onChange event fails after the bounded timeout instead of hanging.
In `@ThawTests/Settings/Models/AutomationSettingsTests.swift`:
- Around line 13-45: Replace real UserDefaults snapshot/restore isolation with
withScratchDefaults in ThawTests/Settings/Models/AutomationSettingsTests.swift
(lines 13-45), removing savedDefaults, init wiping, and deinit restoration and
wrapping each test case body. Apply the same change in
ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift (lines
28-55), moving makeManager and all assertions, including reload cases, inside
withScratchDefaults. In
ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift (lines
17-46), remove snapshot/restore logic, wrap each case in withScratchDefaults,
and update the doc comment to reflect the injectable store.
In `@ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift`:
- Around line 66-100: The fallback tests that depend on display identity should
skip when UUID lookup is unavailable instead of failing via `#require`. Update
resolvedDisplayWithoutOverrideFallsBackToGlobalConfiguration and
activeDisplayUsesExplicitConfiguration to guard their respective Bridging
lookups and return early (or apply the project’s existing skip trait), matching
the early-return pattern in DisplaySettingsManagerMutationTests; leave tests
with valid UUIDs unchanged.
In `@ThawTests/Settings/Models/ExternalSettingsChangeTests.swift`:
- Around line 49-72: Update observerFiltersAndDelivers so its notification
handler cancels subscription before resuming the continuation, preventing
duplicate delivery from resuming it twice or over-counting confirmation. Also
add the .serialized trait to the enclosing External settings changes `@Suite`
declaration to isolate this shared NotificationCenter test suite.
In `@ThawTests/Settings/Models/GeneralSettingsSnapshotTests.swift`:
- Around line 49-52: Update makeCustomSnapshot to require, rather than default,
the .ellipsis and .chevron icon lookups, making the helper throwable so missing
fixtures fail immediately. Propagate this change to both call sites by using try
makeCustomSnapshot() and marking the corresponding tests as throws.
In `@ThawTests/Settings/Models/GeneralSettingsTests.swift`:
- Around line 329-336: Update the three unknown-key external-change tests,
including externalChangeToUnknownKeyIsIgnored and the cases around it, to
compare settings.showIceIcon against Defaults.DefaultValue.showIceIcon instead
of asserting a hard-coded truth value. Preserve the existing ignored-change
verification and test structure.
In `@ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift`:
- Around line 16-27: The hotkey and section-order test seeds currently mutate
global defaults instead of using the scratch store. Update
HotkeysSettingsPersistenceTests.bindingsRoundTrip() and the two
savedSectionOrder setup sites in ProfileManagerUpdateRearmIntegrationTests to
run through withScratchDefaults, moving all seed, read, update, and cleanup
operations to that store and removing direct global snapshot/restore or
UserDefaults.standard writes.
In `@ThawTests/Settings/Models/ProfileApplyInFlightFlagTests.swift`:
- Around line 74-86: Add a `.timeLimit` trait to
`emptyProfileApplyClearsInFlightFlag` so the test fails promptly if
`applyProfileLayout` remains blocked by startup settling. Keep the test’s
existing assertions and empty-layout setup unchanged.
In `@ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift`:
- Around line 458-548: Add the documented `// Format contract: parsed by
ProfileLayoutLogReplayTests.parse(_:)` marker to every production `diagLog` call
site emitting messages matched by `ProfileLayoutLogReplayTests.parse(_:)`,
including the `LittleSnitchOrphanLog` path. Alternatively, remove the parser’s
marker expectation, but preserve an explicit mechanism that prevents rewording
these production log formats from silently breaking fixture parity.
In `@ThawTests/Settings/Models/ProfileManagerCRUDTests.swift`:
- Around line 437-488: Extract the shared makeProfile(named:) fixture and
seedManifest(with:into:) helper into
ThawTests/Support/MenuBarTestFixtures.swift, parameterizing the factory with
name, savedSectionOrder, and displayConfigurations. In
ThawTests/Settings/Models/ProfileManagerCRUDTests.swift ranges 437-488 and
411-435, move these helpers while retaining withManager as the local seam. In
ProfileManagerDeleteTests.swift range 112-192, remove both local copies and use
the shared helpers; in ProfileManagerUpdateRearmIntegrationTests.swift range
136-198, replace makeProfile(savedSectionOrder:) with the shared factory and
retain writeProfile only if needed for manifest-free seeding; in
ProfileExportBundleTests.swift range 29-80, remove makeTestProfile(name:) and
use the shared factory.
In `@ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift`:
- Around line 546-630: The profile-manager test fixtures are duplicated across
both suites. In ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift
lines 546-630, move withTemporaryDirectory (both overloads), seedManifest,
makeEncoder, makeDecoder, and makeProfile into a shared helper under
ThawTests/Support/, retaining only withManager and writeBundle locally. In
ThawTests/Settings/Models/ProfileManagerDeepTests.swift lines 243-341, remove
the duplicate withTemporaryDirectory, seedManifest, makeEncoder, profileURL, and
makeProfile implementations and use the shared helpers, retaining
makeUncreatableDirectoryURL, blockManifestPath, and isDirectory.
In `@ThawTests/Settings/Models/ProfileTests.swift`:
- Around line 548-664: Extract shared file-scope fixture factories for
GeneralSettingsSnapshot and AdvancedSettingsSnapshot, then reuse them in
profileContentInitialization, profileContentWithEmptyCollections, and
ProfileFullTests.makeTestContent(). Preserve each test’s required overrides
while eliminating duplicated initializer argument lists, including the empty
collection overrides for the collection-focused test.
In `@ThawTests/Settings/Search/SearchModelTests.swift`:
- Around line 56-65: Update entryTitlesAreLocalized so it does not assert the
host-dependent entry.titleText value or hard-code the German translation.
Compare titles resolved through the explicitly loaded English and German
bundles, asserting they differ and that the running localization result matches
the appropriate bundle only when the test can determine the active locale.
In `@ThawTests/Settings/URI/IntegrationURIContractTests.swift`:
- Around line 53-60: Add the combined requestId and broadcast URL shape to the
droppyURIs fixture, then update the test around the existing parsing/no-set
assertions to read that URL from the fixture instead of repeating the literal.
Keep droppyURIs as the single source of truth for all Droppy-emitted URL shapes.
In `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift`:
- Around line 29-46: Replace the real UserDefaults snapshot/restore lifecycle in
SettingsURIHandlerApplyTests.swift (lines 29-46) with withScratchDefaults
wrapping each test body. In AdvancedSettingsTests.swift (lines 69-105), remove
touchedKeys, savedDefaults, deinit, and clearStoredSettings, and use
withScratchDefaults so each test starts with an empty isolated store; update
both suites while preserving their existing test behavior.
- Around line 443-452: Rename whitelistChangesAreAnnounced to reflect that it
tests adding an entry, then add a separate removal test using
SettingsURIHandler.removeFromWhitelist after establishing a whitelisted bundle
ID. Assert that the removal operation posts settingsURIWhitelistDidChange,
preserving the existing add notification coverage.
In `@ThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swift`:
- Around line 96-98: Remove the no-op globalBooleanKeys filtering and simplify
everyBooleanKeyIsSettable to use the global .settingsDidChangeViaURI channel
directly, or explicitly include per-display Boolean keys if the per-display
branch and .perDisplaySettingsDidChangeViaURI behavior must remain covered. Keep
the existing perDisplayKeys assertions consistent with the chosen test scope and
update the misleading documentation accordingly.
In `@ThawTests/Settings/URI/SettingsURIHandlerGetTests.swift`:
- Around line 47-64: Remove the whole-domain snapshot and restoration from the
test suite’s init/deinit. Route all suite operations that write through
injectable Defaults storage via withScratchDefaults, and track and restore only
the individual UserDefaults.standard keys written directly by the handler,
preserving values changed by other suites.
In `@ThawTests/Settings/URI/SettingsURIHandlerTailTests.swift`:
- Around line 86-89: Ensure the nested suites EmptyDisplayIdentifier,
UnsupportedPerDisplayToggles, and AnnouncementPayloads execute every
SettingsURIHandler.handleSet and handleToggle call inside withScratchDefaults,
preserving each test’s existing assertions. Alternatively, narrow the suite
documentation to explicitly identify these exemptions, but prefer wrapping the
test bodies so they cannot access or mutate the shared real store.
In `@ThawTests/Settings/URI/SettingsURIParserTests.swift`:
- Around line 21-22: Rename the nested test suite struct from Set to SetRoute
within SettingsURIParserTests, while preserving the `@Suite`("set") display name
and updating any references to the nested type if present.
- Around line 35-46: Update emptyKeyIsNotMalformed in the SettingsURIParserTests
route assertions to pair each URI with its exact expected SettingsURIRoute,
including a present empty key, and compare the parsed route directly rather than
using negative malformed comparisons. Preserve coverage for both set and toggle
URIs.
In `@ThawTests/Shared/DiagnosticLoggerTests.swift`:
- Around line 98-115: Add CaseIterable conformance to DiagnosticLogger.Level in
DiagnosticLogger.swift, then update allLevelsHaveUppercaseRawValues and
allLevelsAreDistinct to use DiagnosticLogger.Level.allCases instead of hardcoded
level arrays.
In `@ThawTests/Shared/WindowInfoTests.swift`:
- Around line 18-43: Update createWindowInfo to be throwing, construct the JSON
payload with Data(json.utf8), and replace the forced JSONDecoder decode with
`#require`. Propagate try to every createWindowInfo call site and mark
non-throwing test methods that invoke it as throws, following the sibling suite
pattern.
In `@ThawTests/Support/ScratchDefaults.swift`:
- Around line 45-65: Document in the withScratchDefaults doc comment that
scratchDefaultsLock is non-recursive and nested calls on the same thread are
unsupported and will deadlock. Do not alter the locking behavior or add
ownership tracking.
- Around line 67-93: In ThawTests/Support/ScratchDefaults.swift lines 67-93,
update withScratchDefaults so saving, installing, and restoring the process-wide
Defaults.store are serialized through an async-safe mutex such as an actor,
while preserving suite cleanup and async body execution. In
ThawTests/Settings/Models/FocusFilterIntentTests.swift lines 57-66, remove the
suite-documentation claim that .serialized makes the async helper safe,
retaining .serialized only for its remaining applicable reasons.
In `@ThawTests/Utilities/CGImageAnalysisTests.swift`:
- Around line 109-119: Strengthen the assertion in explicitColorSpaceIsUsed by
comparing the returned average.colorSpace name directly with CGColorSpace.sRGB
instead of checking only its model. Keep the test’s explicit space argument and
existing setup unchanged so it verifies that averageColor(using:) honors the
requested color space.
In `@ThawTests/Utilities/CGImageDetachedCopyTests.swift`:
- Around line 19-59: Replace the force unwraps in makeSolidImage and pixelData
with try `#require`, and mark both helpers as throws. Update every call site in
the affected tests to use try, including the five references around lines 65,
75, 80, 134, and 140, and mark their enclosing test methods as throws.
In `@ThawTests/Utilities/ConcurrencyHelpersTests.swift`:
- Around line 77-80: Replace the fixed Task.sleep in the cancellation test with
polling of observed until it becomes true or a reasonable deadline expires. Use
the existing observed lock access, include a short interval between polls, and
retain the final expectation so the test tolerates scheduler latency without
waiting indefinitely.
In `@ThawTests/Utilities/ExtensionsGraphicsTests.swift`:
- Around line 17-33: Extract the duplicated premultiplied-first, 32-bit
little-endian bitmap-context builder into a shared fixture under
ThawTests/Support/ and reuse it from
ThawTests/Utilities/ExtensionsGraphicsTests.swift#L17-L33, retaining only the
local fill:background: overload or moving both helpers. Remove
makeImage(width:height:draw:) from
ThawTests/Utilities/CGImageAnalysisTests.swift#L222-L241 and update its callers
to use the shared fixture; also fold makeOpaqueImage in
ThawTests/Utilities/ExtensionsRemainderTests.swift (line range not provided)
into the same fixture, preserving the exact bitmapInfo parameters.
- Around line 482-499: Update visiblePanelWakesTheWaiter to use a Swift Testing
condition trait based on panel.isVisible, so the test is skipped when no window
server is available instead of failing via `#require`. Remove the in-body `#require`
while preserving the existing panel setup, waiter behavior, and visibility
assertion.
In `@ThawTests/Utilities/IceSettingsImporterTests.swift`:
- Around line 13-15: Update IceSettingsImporterTests to match the isolation used
by IceSettingsImporterTailTests: mark the suite as serialized and wrap every
test case’s Defaults-backed work in withScratchDefaults. Ensure all
IceSettingsImporter fixture imports execute against scratch defaults rather than
the process-wide host defaults.
In `@ThawTests/Utilities/MigrationTests.swift`:
- Around line 109-119: Add a non-empty assertion immediately after each
`storedConfigurations()` result is obtained in the migration tests at the
referenced sections, before iterating over `configurations.values`. Keep the
existing per-configuration expectations unchanged, and ensure the tests fail
when the decoded configuration map contains no display entries.
In `@ThawTests/Utilities/ObjectStorageTests.swift`:
- Around line 144-201: Make deallocation explicit in
weakSetDoesNotRetainItsValue and strongWriteCanBeReplacedByAWeakOne instead of
relying on the do-scope ending; use an explicit lifetime-ending action for
payload after the storage write and before asserting observed == nil, while
preserving the existing weak-reference assertions.
In `@ThawTests/Utilities/ShareableContentCacheTests.swift`:
- Around line 78-84: Replace the 20 ms sleeps in both concurrency cases with a
causal synchronization gate: have the first fetch signal when it begins, then
await that signal before starting the subsequent callers. Apply the same gate
before cancelledTask.cancel() in the second case, preserving the existing
fetch-count and cancellation assertions without relying on elapsed time.
In `@ThawTests/Utilities/SimpleSemaphoreTests.swift`:
- Around line 66-105: Correct the reconciliation logic in the semaphore race
loop: in the acquired branch after awaiting waitTask, do not call
semaphore.signal(), while in the timeout branch consume the restored permit with
semaphore.wait(timeout:) so each iteration returns to the held state. Update the
surrounding comments to match this behavior and preserve the final sanity check.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 830cdd91-4f2b-425b-a599-e5d5e90f793c
📒 Files selected for processing (226)
.gitignore.swiftformatMenuBarItemService/Listener.swiftMenuBarItemService/SourcePIDCache.swiftMenuBarItemService/main.swiftShared/Bridging/Shims.swiftShared/Utilities/DiagnosticLogger.swiftShared/Utilities/MarkerPairResolver.swiftThaw/Events/EventTap.swiftThaw/Events/HIDEventManager.swiftThaw/Events/RunLoopLocalEventMonitor.swiftThaw/Hotkeys/Hotkey.swiftThaw/Hotkeys/HotkeyRegistry.swiftThaw/Main/AppState.swiftThaw/Main/Updates.swiftThaw/MenuBar/Appearance/Configurations/MenuBarAppearanceConfigurationV2.swiftThaw/MenuBar/Appearance/Configurations/MenuBarShapes.swiftThaw/MenuBar/Appearance/MissionControlDetector.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/LayoutBar/LayoutBarPaddingView.swiftThaw/MenuBar/MenuBarItems/AXIdentityCatalog.swiftThaw/MenuBar/MenuBarItems/AXItemActivator.swiftThaw/MenuBar/MenuBarItems/ClickReactionVerifier.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swiftThaw/MenuBar/MenuBarItems/MenuBarItem.swiftThaw/MenuBar/MenuBarItems/MenuBarItemFailureLedger.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swiftThaw/MenuBar/MenuBarItems/MenuBarItemTag.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/MenuBarSection.swiftThaw/MenuBar/MenuBarSectionLayout.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Onboarding/Tour/OnboardingTour.swiftThaw/Onboarding/Tour/TourMockups.swiftThaw/Onboarding/Tour/WelcomeMockup.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/FocusFilterIntent.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/UI/Views/CustomTooltip.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/Extensions.swiftThaw/Utilities/ScreenCapture.swiftThaw/Utilities/SettingsURIHandler.swiftThaw/Utilities/SettingsURIParser.swiftThawTests/CaptureBoundsValidationTests.swiftThawTests/CrossCutting/CoverageSweep1Tests.swiftThawTests/CrossCutting/CoverageSweep2Tests.swiftThawTests/CrossCutting/CoverageSweep3Tests.swiftThawTests/CrossCutting/CoverageSweep4Tests.swiftThawTests/CrossCutting/CoverageSweep5Tests.swiftThawTests/DefaultsKeyTests.swiftThawTests/DiagnosticLoggerTests.swiftThawTests/DisplayIceBarConfigurationTests.swiftThawTests/DisplaySettingsManagerSpacingGateTests.swiftThawTests/ExtensionsTests.swiftThawTests/GeneralSettingsSnapshotTests.swiftThawTests/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyLifecycleTests.swiftThawTests/Hotkeys/KeyCodeTests.swiftThawTests/Hotkeys/KeyCombinationTests.swiftThawTests/Hotkeys/ModifiersTests.swiftThawTests/IceBarLocationTests.swiftThawTests/LayoutResetErrorTests.swiftThawTests/MenuBar/Appearance/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBar/Appearance/MenuBarShapesTests.swiftThawTests/MenuBar/Appearance/MenuBarTintKindTests.swiftThawTests/MenuBar/Appearance/MissionControlDetectorTests.swiftThawTests/MenuBar/ControlItem/ControlItemImageTests.swiftThawTests/MenuBar/ControlItem/ControlItemOcclusionTests.swiftThawTests/MenuBar/ControlItem/ControlItemRecoveryTests.swiftThawTests/MenuBar/IceBar/IceBarLocationTests.swiftThawTests/MenuBar/Items/AXIdentityCatalogTests.swiftThawTests/MenuBar/Items/AXItemActivatorTests.swiftThawTests/MenuBar/Items/AnchorDestinationTests.swiftThawTests/MenuBar/Items/BadgeIndexTests.swiftThawTests/MenuBar/Items/CaptureBoundsValidationTests.swiftThawTests/MenuBar/Items/CapturedImageScaleTests.swiftThawTests/MenuBar/Items/ClickReactionVerifierTests.swiftThawTests/MenuBar/Items/ControlCenterHostedMatchLogReplayTests.swiftThawTests/MenuBar/Items/DisplaySpreadGateTests.swiftThawTests/MenuBar/Items/DivergencePersistenceGateTests.swiftThawTests/MenuBar/Items/FlattenCurrentSectionsTests.swiftThawTests/MenuBar/Items/GhostControlItemWindowTests.swiftThawTests/MenuBar/Items/HiddenDragFailureClassificationTests.swiftThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheDisplayResolutionTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheGatingTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheObservationTests.swiftThawTests/MenuBar/Items/MenuBarItemManagerRearmTests.swiftThawTests/MenuBar/Items/MenuBarItemOrderingTests.swiftThawTests/MenuBar/Items/MenuBarItemServiceTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCodingTests.swiftThawTests/MenuBar/Items/MenuBarItemTagTests.swiftThawTests/MenuBar/Items/MenuBarItemValueTests.swiftThawTests/MenuBar/Items/MenuBarTestFixturesTests.swiftThawTests/MenuBar/Items/MidSectionTransitionTests.swiftThawTests/MenuBar/Items/MoveEventCoordinatesTests.swiftThawTests/MenuBar/Items/MoveFailureBackoffTests.swiftThawTests/MenuBar/Items/MoveMenuGuardTests.swiftThawTests/MenuBar/Items/NewItemsPlacementTests.swiftThawTests/MenuBar/Items/PartitionUnmanagedUIDsTests.swiftThawTests/MenuBar/Items/PendingRehideTagIdentifiersTests.swiftThawTests/MenuBar/Items/SavedLayoutSectionLookupTests.swiftThawTests/MenuBar/Items/SavedPositionLookupTests.swiftThawTests/MenuBar/Items/SectionGeometryGateTests.swiftThawTests/MenuBar/Items/SelectWindowForBatchScanTests.swiftThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swiftThawTests/MenuBar/Items/SourcePIDResolutionGateTests.swiftThawTests/MenuBar/Items/WindowIDsChangedGateTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerUnmanagedPlacementTests.swiftThawTests/MenuBar/Layout/LayoutResetErrorTests.swiftThawTests/MenuBar/Layout/LayoutResetReentrancyTests.swiftThawTests/MenuBar/Layout/NotchOverflowRevealTests.swiftThawTests/MenuBar/Layout/PlanFullSortSequenceTests.swiftThawTests/MenuBar/Layout/PlanLCSMoveSequenceTests.swiftThawTests/MenuBar/Layout/PlanLeftmostMoveTests.swiftThawTests/MenuBar/Layout/PlanNotchOverflowTests.swiftThawTests/MenuBar/Layout/PlanPendingMoveTests.swiftThawTests/MenuBar/Layout/PlanSectionOrderTests.swiftThawTests/MenuBar/Layout/PlanUnmanagedPlacementTests.swiftThawTests/MenuBar/Section/MenuBarSectionGeometryTests.swiftThawTests/MenuBar/Section/MenuBarSectionNameTests.swiftThawTests/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBarItemServiceTests.swiftThawTests/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBarItemTagTests.swiftThawTests/MenuBarSectionNameTests.swiftThawTests/MenuBarShapesTests.swiftThawTests/MenuBarTestFixturesTests.swiftThawTests/ModifiersTests.swiftThawTests/MoveFailureBackoffTests.swiftThawTests/Onboarding/OnboardingMockupsTests.swiftThawTests/Onboarding/OnboardingSlideTests.swiftThawTests/OnboardingMockupsTests.swiftThawTests/OnboardingSlideTests.swiftThawTests/PermissionTests.swiftThawTests/Permissions/PermissionTests.swiftThawTests/ProfileEntityTests.swiftThawTests/ProfileManagerRearmGateTests.swiftThawTests/RehideStrategyTests.swiftThawTests/SectionDividerStyleTests.swiftThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/AutomationSettingsTests.swiftThawTests/Settings/Models/DisplayIceBarConfigurationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/Settings/Models/DisplaySettingsManagerLookupTests.swiftThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerSpacingGateTests.swiftThawTests/Settings/Models/DisplaySettingsManagerURINotificationTests.swiftThawTests/Settings/Models/ExternalSettingsChangeTests.swiftThawTests/Settings/Models/FocusFilterIntentTests.swiftThawTests/Settings/Models/GeneralSettingsSnapshotTests.swiftThawTests/Settings/Models/GeneralSettingsTests.swiftThawTests/Settings/Models/HookScriptTests.swiftThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swiftThawTests/Settings/Models/ProfileApplyInFlightFlagTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/Models/ProfileEntityTests.swiftThawTests/Settings/Models/ProfileExportBundleTests.swiftThawTests/Settings/Models/ProfileLayoutLogReplayTests.swiftThawTests/Settings/Models/ProfileManagerCRUDTests.swiftThawTests/Settings/Models/ProfileManagerDeepTests.swiftThawTests/Settings/Models/ProfileManagerDeleteTests.swiftThawTests/Settings/Models/ProfileManagerPersistenceTests.swiftThawTests/Settings/Models/ProfileManagerRearmGateTests.swiftThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swiftThawTests/Settings/Models/ProfileTests.swiftThawTests/Settings/Models/RehideStrategyTests.swiftThawTests/Settings/Models/SectionDividerStyleTests.swiftThawTests/Settings/Models/SettingsResetterTests.swiftThawTests/Settings/Search/SearchIndexTests.swiftThawTests/Settings/Search/SearchModelTests.swiftThawTests/Settings/Search/SettingsSearchNavigationTests.swiftThawTests/Settings/URI/IntegrationURIContractTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Settings/URI/SettingsURIHandlerGetTests.swiftThawTests/Settings/URI/SettingsURIHandlerTailTests.swiftThawTests/Settings/URI/SettingsURIHandlerTests.swiftThawTests/Settings/URI/SettingsURIParserTests.swiftThawTests/SettingsResetterTests.swiftThawTests/SettingsURIHandlerTests.swiftThawTests/Shared/CodeSigningInfoTests.swiftThawTests/Shared/DiagnosticLoggerFileTests.swiftThawTests/Shared/DiagnosticLoggerTests.swiftThawTests/Shared/MarkerPairResolverTests.swiftThawTests/Shared/SharedExtensionsTests.swiftThawTests/Shared/WindowInfoDecodingTests.swiftThawTests/Shared/WindowInfoDerivedTests.swiftThawTests/Shared/WindowInfoTests.swiftThawTests/SharedExtensionsTests.swiftThawTests/Support/MenuBarTestFixtures.swiftThawTests/Support/ScratchDefaults.swiftThawTests/Utilities/CGImageAnalysisTests.swiftThawTests/Utilities/CGImageDetachedCopyTests.swiftThawTests/Utilities/ConcurrencyHelpersTests.swiftThawTests/Utilities/CustomTooltipPanelTests.swiftThawTests/Utilities/DefaultsKeyTests.swiftThawTests/Utilities/ExtensionsCoverageTests.swiftThawTests/Utilities/ExtensionsGraphicsTests.swiftThawTests/Utilities/ExtensionsRemainderTests.swiftThawTests/Utilities/ExtensionsTests.swiftThawTests/Utilities/HookRunnerTests.swiftThawTests/Utilities/IceSettingsImporterTailTests.swiftThawTests/Utilities/IceSettingsImporterTests.swiftThawTests/Utilities/LocalizedErrorWrapperTests.swiftThawTests/Utilities/MigrationTests.swiftThawTests/Utilities/MouseMovedThrottleTests.swiftThawTests/Utilities/ObjectStorageTests.swiftThawTests/Utilities/PredicatesTests.swiftThawTests/Utilities/ShareableContentCacheTests.swiftThawTests/Utilities/SimpleSemaphoreTests.swiftdocs/ASSURANCE_CASE.mdscripts/coverage-report.pyscripts/swiftlint-inputs.xcfilelistsonar-project.properties
💤 Files with no reviewable changes (29)
- ThawTests/IceBarLocationTests.swift
- ThawTests/DiagnosticLoggerTests.swift
- ThawTests/HotkeyActionTests.swift
- Thaw/MenuBar/Appearance/MissionControlDetector.swift
- ThawTests/MenuBarSectionNameTests.swift
- ThawTests/MenuBarTestFixturesTests.swift
- ThawTests/CaptureBoundsValidationTests.swift
- ThawTests/MenuBarShapesTests.swift
- ThawTests/MoveFailureBackoffTests.swift
- ThawTests/MenuBarItemTagTests.swift
- ThawTests/OnboardingMockupsTests.swift
- ThawTests/ProfileManagerRearmGateTests.swift
- ThawTests/OnboardingSlideTests.swift
- ThawTests/ExtensionsTests.swift
- ThawTests/DisplaySettingsManagerSpacingGateTests.swift
- ThawTests/DefaultsKeyTests.swift
- ThawTests/LayoutResetErrorTests.swift
- ThawTests/MenuBarItemTagCanonicalizationTests.swift
- ThawTests/MenuBarItemServiceTests.swift
- ThawTests/DisplayIceBarConfigurationTests.swift
- ThawTests/RehideStrategyTests.swift
- ThawTests/MenuBar/Items/GhostControlItemWindowTests.swift
- ThawTests/PermissionTests.swift
- ThawTests/MenuBarAppearanceConfigurationTests.swift
- ThawTests/ModifiersTests.swift
- ThawTests/GeneralSettingsSnapshotTests.swift
- ThawTests/SectionDividerStyleTests.swift
- Thaw/MenuBar/MenuBarSection.swift
- ThawTests/ProfileEntityTests.swift
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (22)
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (1)
7015-7017: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restore state after a cancelled profile apply.
This check only handles cancellation before
armProfileState. A cancellation during Phase 6 reaches Phase 7. The code skips direct persistence, but it retains the newsavedSectionOrder, pinning sets, andactiveProfileLayout.The deferred cache refresh can then save a partial layout. A later late-arrival re-sort can also apply the cancelled profile state.
Snapshot the prior state before arming. Use an apply token to restore it only when the cancelled operation still owns the state. Do not schedule the deferred refresh after cancellation.
🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 7015 - 7017, Update the profile-apply flow surrounding armProfileState to snapshot the prior savedSectionOrder, pinning sets, and activeProfileLayout and associate the operation with an apply token; when cancellation is detected in Phase 6 or Phase 7, restore the snapshot only if that token still owns the state, prevent late-arrival re-sorting from applying the cancelled profile, and skip scheduling the deferred cache refresh.ThawTests/Settings/Models/ExternalSettingsChangeTests.swift (1)
49-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the continuation against a second resume, and isolate the observer from other suites.
The handler resumes the continuation on every delivered change. Two conditions make a second delivery possible:
- The subscription stays active until Line 71, after the
confirmationblock returns.- The observer listens on
NotificationCenter.default, which every suite shares. This suite is not.serialized, and Swift Testing runs suites in parallel, so another suite that posts.settingsDidChangeViaURIreaches this handler.A second delivery resumes the same
CheckedContinuationtwice, which traps at runtime, and it also over-counts the confirmation. Cancel the subscription inside the handler before resuming, and mark the suite.serialized.🛡️ Proposed fix
`@Suite`("External settings changes") +@MainActor +// Serialized: the observer listens on the shared `NotificationCenter.default`, +// which other settings suites also post to. struct ExternalSettingsChangeTests {`@Test`("The observer ignores malformed notifications and delivers valid changes") func observerFiltersAndDelivers() async { var subscription: AnyCancellable? await confirmation("Delivered one valid settings change") { confirm in await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in subscription = NotificationCenter.observeSettingsChangesViaURI { change in + guard change.key == "showOnHover" else { return } + // Cancel first: a later delivery must not resume twice. + subscription?.cancel() + subscription = nil `#expect`(change.key == "showOnHover") `#expect`(change.boolValue == true) confirm() continuation.resume() }Apply
.serializedon the@Suiteattribute:`@Suite`("External settings changes", .serialized)🤖 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 `@ThawTests/Settings/Models/ExternalSettingsChangeTests.swift` around lines 49 - 72, Update observerFiltersAndDelivers so its notification handler cancels subscription before resuming the continuation, preventing duplicate delivery from resuming it twice or over-counting confirmation. Also add the .serialized trait to the enclosing External settings changes `@Suite` declaration to isolate this shared NotificationCenter test suite.ThawTests/Settings/Models/GeneralSettingsTests.swift (1)
329-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Assert against
Defaults.DefaultValue.showIceIcon.These three tests verify that a change is ignored. They assert
settings.showIceIconas a bare truth value, which also encodes the current shipped default as a literal.If
Defaults.DefaultValue.showIceIconbecomesfalse, all three fail even though the ignore behaviour is still correct. The neighbouring tests at lines 348-349 already use theDefaults.DefaultValueform.♻️ Proposed change
- `#expect`(settings.showIceIcon, "\(key) must not reach any property") + `#expect`( + settings.showIceIcon == Defaults.DefaultValue.showIceIcon, + "\(key) must not reach any property" + )Also applies to: 376-384, 397-405
🤖 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 `@ThawTests/Settings/Models/GeneralSettingsTests.swift` around lines 329 - 336, Update the three unknown-key external-change tests, including externalChangeToUnknownKeyIsIgnored and the cases around it, to compare settings.showIceIcon against Defaults.DefaultValue.showIceIcon instead of asserting a hard-coded truth value. Preserve the existing ignored-change verification and test structure.ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift (1)
16-27: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -H 'ScratchDefaults.swift' --exec cat -n rg -nP -C 3 'savedSectionOrder' Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift rg -nP 'UserDefaults\.standard' --type=swift -g '!ThawTests/**'Repository: thaw-app/Thaw
Length of output: 19898
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Candidate files ==" fd -i 'Defaults.swift|HotkeysSettingsPersistenceTests.swift|ProfileManagerUpdateRearmIntegrationTests.swift|MenuBarItemManager.swift|ScratchDefaults.swift' . echo echo "== Defaults implementation/declarations ==" for f in $(fd -i 'Defaults.swift' .); do echo "-- $f --" wc -l "$f" sed -n '1,220p' "$f" | cat -n done echo echo "== Hotkeys test section ==" f=$(fd -i 'HotkeysSettingsPersistenceTests.swift' . | head -n1) echo "-- $f --" wc -l "$f" sed -n '1,120p' "$f" | cat -n echo echo "== Profile rearm test section ==" f=$(fd -i 'ProfileManagerUpdateRearmIntegrationTests.swift' . | head -n1) echo "-- $f --" wc -l "$f" sed -n '1,160p' "$f" | cat -n rg -n -C 4 'savedSectionOrder|Defaults|withScratchDefaults|`@Test`|func ' "$f" echo echo "== Hotkeys access through Defaults (no tests) ==" rg -n -C 3 'Defaults\.|`@objc` dynamic\.|\.hotkeys|HotkeyAction' --type=swift -g '!ThawTests/**' Thaw | head -n 200Repository: thaw-app/Thaw
Length of output: 45576
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Remaining UserDefaults.standard references in non-test Swift files ==" rg -n -C 2 'UserDefaults\.standard' --type=swift -g '!ThawTests/**' || true echo echo "== ScratchDefaults import paths ==" rg -n 'ScratchDefaults|withScratchDefaults' --type=swift -g '!ThawTests/**' || trueRepository: thaw-app/Thaw
Length of output: 224
Route both test suites through
withScratchDefaults.HotkeysSettingsPersistenceTests.bindingsRoundTrip()still snapshots and restores.hotkeyson the global store, andProfileManagerRearmIntegrationTestsseedssavedSectionOrderdirectly intoUserDefaults.standard; move the hotkeys seed and both section-order seeds through a scratch store so the real defaults domain is not a crash/uninterruptible teardown hazard.📍 Affects 2 files
ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift#L16-L27(this comment)ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift#L27-L49🤖 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 `@ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift` around lines 16 - 27, The hotkey and section-order test seeds currently mutate global defaults instead of using the scratch store. Update HotkeysSettingsPersistenceTests.bindingsRoundTrip() and the two savedSectionOrder setup sites in ProfileManagerUpdateRearmIntegrationTests to run through withScratchDefaults, moving all seed, read, update, and cleanup operations to that store and removing direct global snapshot/restore or UserDefaults.standard writes.ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift (1)
458-548: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check the format-contract markers and the checked-in log fixture. set -uo pipefail echo "== format-contract markers ==" rg -n 'Format contract: parsed by ProfileLayoutLogReplayTests' -g '!ThawTests/**' || echo "NO MARKERS FOUND" echo echo "== production message texts the parser matches ==" for pat in \ 'Missing sourcePID for' \ 'applyProfileLayout: current ' \ 'Profile layout Phase 1: ahCtrlUID=' \ 'Profile layout Phase 1: desiredHidden=' \ 'Profile layout Phase 1: desiredAH=' \ 'Profile layout Phase 1: desiredVisible=' \ 'Profile layout: planUnmanagedPlacement' \ 'Notch overflow budget:' \ 'notch overflow; ' ; do printf '\n--- %s ---\n' "$pat" rg -n --fixed-strings "$pat" -g '!ThawTests/**' || echo "NOT FOUND IN PRODUCTION SOURCES" done echo echo "== fixture ==" rg -n 'enum LittleSnitchOrphanLog|LittleSnitchOrphanLog' -g '*.swift' | head -20Repository: thaw-app/Thaw
Length of output: 198
🏁 Script executed:
#!/bin/bash # Description: Inspect the parser, surrounding test fixtures, and relevant production references. set -uo pipefail echo "== candidate file list ==" git ls-files | rg 'ProfileLayoutLogReplayTests|LittleSnitchOrphanLog|ProfileLayout|Layout' | head -200 echo echo "== parser file outline/size ==" FILE="$(git ls-files | rg 'ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift$' | head -1)" echo "FILE=$FILE" if [ -n "$FILE" ]; then wc -l "$FILE" sed -n '420,570p' "$FILE" | cat -n fi echo echo "== all references to parser and fixture ==" rg -n 'ProfileLayoutLogReplayTests\.parse|parse\(_:\)|LittleSnitchOrphanLog|Profile layout Phase 1:|Missing sourcePID|planUnmanagedPlacement|notch overflow|applyProfileLayout: current' -g '*.swift' | head -300Repository: thaw-app/Thaw
Length of output: 9646
Add the missing format-contract markers before relying on this parser.
The
LittleSnitchOrphanLogfixture is present, but none of the production call sites carry the documented// Format contract: parsed by ProfileLayoutLogReplayTests.parse(_:)marker that this parser depends on. Add the marker or remove this contract expectation to keep log-message rewording from producing silent parity errors.🤖 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 `@ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift` around lines 458 - 548, Add the documented `// Format contract: parsed by ProfileLayoutLogReplayTests.parse(_:)` marker to every production `diagLog` call site emitting messages matched by `ProfileLayoutLogReplayTests.parse(_:)`, including the `LittleSnitchOrphanLog` path. Alternatively, remove the parser’s marker expectation, but preserve an explicit mechanism that prevents rewording these production log formats from silently breaking fixture parity.ThawTests/Settings/Models/ProfileManagerCRUDTests.swift (1)
437-488: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the profile fixture and the manifest-seeding helper into
ThawTests/Support. Four suites each carry a private copy of the same ~50-lineProfileContentfixture, and two carry a byte-identicalseedManifest.ProfileContent,GeneralSettingsSnapshot, andAdvancedSettingsSnapshotare memberwise-initialized, so every field added to any of them breaks all four copies at once, and a copy that is updated differently from the others silently changes what its suite asserts.ThawTests/Support/MenuBarTestFixtures.swiftalready exists in this PR as the home for shared fixtures.
ThawTests/Settings/Models/ProfileManagerCRUDTests.swift#L437-L488: movemakeProfile(named:)intoThawTests/Supportas the single fixture factory, parameterized onnameand on the fields the other suites vary (savedSectionOrder,displayConfigurations).ThawTests/Settings/Models/ProfileManagerCRUDTests.swift#L411-L435: moveseedManifest(with:into:)intoThawTests/Supportalongside it, and keepwithManagerhere as the local seam over it.ThawTests/Settings/Models/ProfileManagerDeleteTests.swift#L112-L192: delete both local helpers and call the shared factory and the sharedseedManifest.ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift#L136-L198: deletemakeProfile(savedSectionOrder:)and call the shared factory with thesavedSectionOrderargument; keepwriteProfileonly if seeding without a manifest is required here.ThawTests/Settings/Models/ProfileExportBundleTests.swift#L29-L80: deletemakeTestProfile(name:)and call the shared factory.📍 Affects 4 files
ThawTests/Settings/Models/ProfileManagerCRUDTests.swift#L437-L488(this comment)ThawTests/Settings/Models/ProfileManagerCRUDTests.swift#L411-L435ThawTests/Settings/Models/ProfileManagerDeleteTests.swift#L112-L192ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift#L136-L198ThawTests/Settings/Models/ProfileExportBundleTests.swift#L29-L80🤖 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 `@ThawTests/Settings/Models/ProfileManagerCRUDTests.swift` around lines 437 - 488, Extract the shared makeProfile(named:) fixture and seedManifest(with:into:) helper into ThawTests/Support/MenuBarTestFixtures.swift, parameterizing the factory with name, savedSectionOrder, and displayConfigurations. In ThawTests/Settings/Models/ProfileManagerCRUDTests.swift ranges 437-488 and 411-435, move these helpers while retaining withManager as the local seam. In ProfileManagerDeleteTests.swift range 112-192, remove both local copies and use the shared helpers; in ProfileManagerUpdateRearmIntegrationTests.swift range 136-198, replace makeProfile(savedSectionOrder:) with the shared factory and retain writeProfile only if needed for manifest-free seeding; in ProfileExportBundleTests.swift range 29-80, remove makeTestProfile(name:) and use the shared factory.ThawTests/Settings/Search/SearchModelTests.swift (1)
56-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test depends on the host locale and on an exact translation string.
Line 62 asserts
entry.titleText == "Launch at Login".titleTextresolves against the running localization, so the test fails on a machine or CI runner whose preferred language is German. Line 64 also pins the exact German string, so any translation edit breaks the test. Compare the two bundle-resolved titles against each other, or derive the expected values from the same.stringsresources instead of hard-coding them.♻️ Suggested direction
- `#expect`(entry.titleText == "Launch at Login") `#expect`(entry.localizedTitle(bundle: english) == "Launch at Login") - `#expect`(entry.localizedTitle(bundle: german) == "Beim Einloggen starten") + `#expect`(entry.localizedTitle(bundle: german) != entry.localizedTitle(bundle: english))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test`("Entry titles resolve to the running localization, not the English source") func entryTitlesAreLocalized() throws { let entry = try `#require`(SearchIndex.entries.first { $0.id == "general.launchAtLogin" }) let german = try Self.localizationBundle("de") let english = try Self.localizationBundle("en") `#expect`(entry.localizedTitle(bundle: english) == "Launch at Login") `#expect`(entry.localizedTitle(bundle: german) != entry.localizedTitle(bundle: english)) }🤖 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 `@ThawTests/Settings/Search/SearchModelTests.swift` around lines 56 - 65, Update entryTitlesAreLocalized so it does not assert the host-dependent entry.titleText value or hard-code the German translation. Compare titles resolved through the explicitly loaded English and German bundles, asserting they differ and that the running localization result matches the appropriate bundle only when the test can determine the active locale.ThawTests/Settings/URI/IntegrationURIContractTests.swift (1)
53-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add the combined
requestIdplusbroadcastshape todroppyURIs.Line 52 states the list holds every URL shape Droppy can emit. The test at line 148 parses
thaw://get?key=all&callback=droppy://thaw-response&requestId=7&broadcast=true, which the list does not contain. That URL therefore never reaches the malformed guard at line 74 or the no-set guard at line 165.Add the combined shape to the list and read it back in the test, so the fixture list stays the single source of truth.
♻️ Proposed change to keep the fixture list authoritative
private let droppyURIs: [String] = [ "thaw://get?key=all&callback=droppy://thaw-response", "thaw://get?key=all&callback=droppy%3A%2F%2Fthaw-response", "thaw://get?key=all&callback=droppy://thaw-response&requestId=7", "thaw://get?key=all&callback=droppy://thaw-response&broadcast=true", + "thaw://get?key=all&callback=droppy://thaw-response&requestId=7&broadcast=true", "thaw://toggle?key=autoRehide", "thaw://toggle?key=useIceBar", ]Then reference that entry from the test instead of repeating the literal:
`@Test`("Request correlation and broadcast are carried through") func requestIdAndBroadcast() throws { - let uri = "thaw://get?key=all&callback=droppy://thaw-response&requestId=7&broadcast=true" + let uri = try `#require`(droppyURIs.first { $0.contains("requestId=7&broadcast=true") })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private let droppyURIs: [String] = [ "thaw://get?key=all&callback=droppy://thaw-response", "thaw://get?key=all&callback=droppy%3A%2F%2Fthaw-response", "thaw://get?key=all&callback=droppy://thaw-response&requestId=7", "thaw://get?key=all&callback=droppy://thaw-response&broadcast=true", "thaw://get?key=all&callback=droppy://thaw-response&requestId=7&broadcast=true", "thaw://toggle?key=autoRehide", "thaw://toggle?key=useIceBar", ]🤖 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 `@ThawTests/Settings/URI/IntegrationURIContractTests.swift` around lines 53 - 60, Add the combined requestId and broadcast URL shape to the droppyURIs fixture, then update the test around the existing parsing/no-set assertions to read that URL from the fixture instead of repeating the literal. Keep droppyURIs as the single source of truth for all Droppy-emitted URL shapes.ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift (2)
29-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two suites mutate the real defaults domain instead of the injectable scratch store. The shared root cause is that both suites drive production code against
UserDefaults.standardand then try to undo the damage indeinit. The PR adds injectableDefaultsstorage, andSettingsURIHandlerCoverageTestsalready drives the sameSettingsURIHandlerentirely insidewithScratchDefaults, so the seam is available for both. Snapshot/restore also races with parallel suites, whichAdvancedSettingsTestsdocuments at lines 32-36 and 358-361.
ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift#L29-L46: replace thepersistentDomain/setPersistentDomainsnapshot anddeinitwithwithScratchDefaultsaround each test body. This suite performs the whole-domain snapshot thatAdvancedSettingsTestswarns can capture and then write back another suite's scratch values.ThawTests/Settings/Models/AdvancedSettingsTests.swift#L69-L105: replacetouchedKeys,savedDefaults,deinit, andclearStoredSettingswithwithScratchDefaults, which starts every test from an empty suite without touching the developer's settings.📍 Affects 2 files
ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift#L29-L46(this comment)ThawTests/Settings/Models/AdvancedSettingsTests.swift#L69-L105🤖 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 `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift` around lines 29 - 46, Replace the real UserDefaults snapshot/restore lifecycle in SettingsURIHandlerApplyTests.swift (lines 29-46) with withScratchDefaults wrapping each test body. In AdvancedSettingsTests.swift (lines 69-105), remove touchedKeys, savedDefaults, deinit, and clearStoredSettings, and use withScratchDefaults so each test starts with an empty isolated store; update both suites while preserving their existing test behavior.
443-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The test name says "Removing", but the body only adds.
whitelistChangesAreAnnouncedasserts the notification posted byaddToWhitelist. It never callsremoveFromWhitelist, so the removal notification documented inThaw/Utilities/SettingsURIHandler.swift(lines 339-351) stays untested. Rename the test to match the body, and add the removal case.💚 Proposed fix
- `@Test`("Removing an entry announces the change") - func whitelistChangesAreAnnounced() { + `@Test`("Adding an entry announces the change") + func whitelistAdditionIsAnnounced() { Defaults.removeObject(forKey: .settingsURIWhitelist) let posted = notifications(named: .settingsURIWhitelistDidChange) { SettingsURIHandler.addToWhitelist(bundleId: "com.example.Alpha") } `#expect`(!posted.isEmpty) } + + `@Test`("Removing an entry announces the change") + func whitelistRemovalIsAnnounced() { + Defaults.removeObject(forKey: .settingsURIWhitelist) + SettingsURIHandler.addToWhitelist(bundleId: "com.example.Alpha") + + let posted = notifications(named: .settingsURIWhitelistDidChange) { + SettingsURIHandler.removeFromWhitelist(bundleId: "com.example.Alpha") + } + + `#expect`(!posted.isEmpty) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test`("Adding an entry announces the change") func whitelistAdditionIsAnnounced() { Defaults.removeObject(forKey: .settingsURIWhitelist) let posted = notifications(named: .settingsURIWhitelistDidChange) { SettingsURIHandler.addToWhitelist(bundleId: "com.example.Alpha") } `#expect`(!posted.isEmpty) } `@Test`("Removing an entry announces the change") func whitelistRemovalIsAnnounced() { Defaults.removeObject(forKey: .settingsURIWhitelist) SettingsURIHandler.addToWhitelist(bundleId: "com.example.Alpha") let posted = notifications(named: .settingsURIWhitelistDidChange) { SettingsURIHandler.removeFromWhitelist(bundleId: "com.example.Alpha") } `#expect`(!posted.isEmpty) }🤖 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 `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift` around lines 443 - 452, Rename whitelistChangesAreAnnounced to reflect that it tests adding an entry, then add a separate removal test using SettingsURIHandler.removeFromWhitelist after establishing a whitelisted bundle ID. Assert that the removal operation posts settingsURIWhitelistDidChange, preserving the existing add notification coverage.ThawTests/Settings/URI/SettingsURIHandlerGetTests.swift (1)
47-64: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not snapshot and restore the whole persistent domain.
initcaptures the entire persistent domain, anddeinitwrites it back..serializedorders cases inside this suite only; it does not serialize this suite against other suites.
ThawTests/Settings/Models/GeneralSettingsTests.swift:31-35documents this exact hazard, and lines 308-311 of that file already weaken an assertion because of it: "a suite that restores a whole persistent domain can land during the suspension above and wipe the write this model just made." This suite is that suite.Two consequences follow. Concurrently running suites lose scratch values that this restore overwrites, and values another suite wrote after this snapshot are silently reverted.
This PR adds injectable
Defaultsstorage. Route the suite throughwithScratchDefaultsfor everything it writes throughDefaults, and restore only the individual keys the handler writes straight toUserDefaults.standard.🤖 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 `@ThawTests/Settings/URI/SettingsURIHandlerGetTests.swift` around lines 47 - 64, Remove the whole-domain snapshot and restoration from the test suite’s init/deinit. Route all suite operations that write through injectable Defaults storage via withScratchDefaults, and track and restore only the individual UserDefaults.standard keys written directly by the handler, preserving values changed by other suites.ThawTests/Settings/URI/SettingsURIParserTests.swift (2)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename the nested
Setsuite.
struct SetshadowsSwift.Setfor every declaration nested insideSettingsURIParserTests. Nothing in the file usesSwift.Settoday, so it compiles. A later edit that writesSet(...)in this file resolves to the test suite type and produces a confusing error. Name itSetRouteand keep the@Suite("set")display name.♻️ Proposed refactor
`@Suite`("set") - struct Set { + struct SetRoute {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Suite`("set") struct SetRoute {🤖 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 `@ThawTests/Settings/URI/SettingsURIParserTests.swift` around lines 21 - 22, Rename the nested test suite struct from Set to SetRoute within SettingsURIParserTests, while preserving the `@Suite`("set") display name and updating any references to the nested type if present.
35-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the exact route instead of two negative comparisons.
The stated intent is that
key=yields a present but empty key. The current assertions only rule out two.malformedvalues. A parser that returned.unrecognized(host: "set"), or.setwith the key dropped entirely, would still pass both checks.Pair each URI with its expected route and compare directly.
♻️ Proposed refactor
`@Test`("An empty key is present, not absent", arguments: [ - "thaw://set?key=&value=true", - "thaw://toggle?key=", + ("thaw://set?key=&value=true", SettingsURIRoute.set(key: "", value: "true", displayUUID: nil)), + ("thaw://toggle?key=", SettingsURIRoute.toggle(key: "", displayUUID: nil)), ]) - func emptyKeyIsNotMalformed(uri: String) throws { - let route = try parse(uri).route - `#expect`(route != .malformed(host: "set")) - `#expect`(route != .malformed(host: "toggle")) + func emptyKeyIsNotMalformed(uri: String, expected: SettingsURIRoute) throws { + `#expect`(try parse(uri).route == expected, "\(uri)") }Adjust the route type name if it differs from
SettingsURIRoute.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// `key=` supplies an empty value rather than omitting the parameter, so /// it parses and is rejected later by key validation. ThawCtl and Droppy /// both emit this shape, so it must not be treated as malformed. `@Test`("An empty key is present, not absent", arguments: [ ("thaw://set?key=&value=true", SettingsURIRoute.set(key: "", value: "true", displayUUID: nil)), ("thaw://toggle?key=", SettingsURIRoute.toggle(key: "", displayUUID: nil)), ]) func emptyKeyIsNotMalformed(uri: String, expected: SettingsURIRoute) throws { `#expect`(try parse(uri).route == expected, "\(uri)") }🤖 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 `@ThawTests/Settings/URI/SettingsURIParserTests.swift` around lines 35 - 46, Update emptyKeyIsNotMalformed in the SettingsURIParserTests route assertions to pair each URI with its exact expected SettingsURIRoute, including a present empty key, and compare the parsed route directly rather than using negative malformed comparisons. Preserve coverage for both set and toggle URIs.ThawTests/Shared/WindowInfoTests.swift (1)
18-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the fixture helper throw instead of trapping.
try!on line 42 traps ifWindowInfodecoding ever fails. A trap aborts the whole test process, so a single coding-key change inWindowInfowould stop every other suite in the run instead of failing these tests.Mark the helper
throwsand use#require, which is the pattern the sibling suites in this PR already use.Data(json.utf8)also removes the force unwrap on line 41.♻️ Proposed change
- ) -> WindowInfo { + ) throws -> WindowInfo { @@ - let data = json.data(using: .utf8)! - return try! JSONDecoder().decode(WindowInfo.self, from: data) + return try JSONDecoder().decode(WindowInfo.self, from: Data(json.utf8)) }Every call site then needs
try, and the non-throwing tests need athrowsannotation.🤖 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 `@ThawTests/Shared/WindowInfoTests.swift` around lines 18 - 43, Update createWindowInfo to be throwing, construct the JSON payload with Data(json.utf8), and replace the forced JSONDecoder decode with `#require`. Propagate try to every createWindowInfo call site and mark non-throwing test methods that invoke it as throws, following the sibling suite pattern.ThawTests/Utilities/CGImageAnalysisTests.swift (1)
109-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Strengthen the explicit color-space assertion.
Line 118 asserts only
model == .rgb. The fallback chain inaverageColoralso produces an RGB space, throughself.colorSpace, then displayP3, then device RGB. So this assertion passes even if thespaceargument is dropped, and the test does not verify what its name claims.Compare the returned color space name to
CGColorSpace.sRGBinstead.♻️ Proposed change to pin the explicit color space
let average = try `#require`(image.averageColor(using: space)) - `#expect`(average.colorSpace?.model == .rgb) + `#expect`(average.colorSpace?.name == CGColorSpace.sRGB)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test`("An explicit RGB color space is honored") func explicitColorSpaceIsUsed() throws { let image = try makeImage(width: 4, height: 4) { context in context.setFillColor(red: 0.5, green: 0.5, blue: 0.5, alpha: 1) context.fill(CGRect(x: 0, y: 0, width: 4, height: 4)) } let space = try `#require`(CGColorSpace(name: CGColorSpace.sRGB)) let average = try `#require`(image.averageColor(using: space)) `#expect`(average.colorSpace?.name == CGColorSpace.sRGB) }🤖 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 `@ThawTests/Utilities/CGImageAnalysisTests.swift` around lines 109 - 119, Strengthen the assertion in explicitColorSpaceIsUsed by comparing the returned average.colorSpace name directly with CGColorSpace.sRGB instead of checking only its model. Keep the test’s explicit space argument and existing setup unchanged so it verifies that averageColor(using:) honors the requested color space.ThawTests/Utilities/CGImageDetachedCopyTests.swift (1)
19-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the force unwraps in the helpers with
try#require``.Lines 30, 38, and 56 force-unwrap
CGContext(...)andmakeImage(). A nil result crashes the test process and takes down every other suite in the run, instead of failing one test with a message.Every comparable helper added in this PR already uses
try#require`` for these same calls. SeeThawTests/Utilities/CGImageAnalysisTests.swiftlines 227-240 and `ThawTests/Utilities/ExtensionsGraphicsTests.swift` lines 18-32.Make both helpers
throwsand update the four call sites at lines 65, 75, 80, 134, and 140.🛡️ Proposed fix for the helpers
- private func makeSolidImage(width: Int, height: Int, color: (UInt8, UInt8, UInt8, UInt8)) -> CGImage { + private func makeSolidImage(width: Int, height: Int, color: (UInt8, UInt8, UInt8, UInt8)) throws -> CGImage { let colorSpace = CGColorSpaceCreateDeviceRGB() let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue - let context = CGContext( - data: nil, - width: width, - height: height, - bitsPerComponent: 8, - bytesPerRow: 0, - space: colorSpace, - bitmapInfo: bitmapInfo - )! + let context = try `#require`( + CGContext( + data: nil, + width: width, + height: height, + bitsPerComponent: 8, + bytesPerRow: 0, + space: colorSpace, + bitmapInfo: bitmapInfo + ), + "Could not create a bitmap context" + ) context.setFillColor( red: CGFloat(color.0) / 255, green: CGFloat(color.1) / 255, blue: CGFloat(color.2) / 255, alpha: CGFloat(color.3) / 255 ) context.fill(CGRect(x: 0, y: 0, width: width, height: height)) - return context.makeImage()! + return try `#require`(context.makeImage(), "Could not snapshot the bitmap context") } /// Reads the raw RGBA bytes of `image` into an array. - private func pixelData(of image: CGImage) -> [UInt8] { + private func pixelData(of image: CGImage) throws -> [UInt8] { let width = image.width let height = image.height var data = [UInt8](repeating: 0, count: width * height * 4) let colorSpace = CGColorSpaceCreateDeviceRGB() let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue - let context = CGContext( - data: &data, - width: width, - height: height, - bitsPerComponent: 8, - bytesPerRow: width * 4, - space: colorSpace, - bitmapInfo: bitmapInfo - )! + let context = try `#require`( + CGContext( + data: &data, + width: width, + height: height, + bitsPerComponent: 8, + bytesPerRow: width * 4, + space: colorSpace, + bitmapInfo: bitmapInfo + ), + "Could not create a bitmap context" + ) context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height)) return data }Mark the three call-site tests
throwsand addtryat lines 65, 75, 80, 134, and 140.🤖 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 `@ThawTests/Utilities/CGImageDetachedCopyTests.swift` around lines 19 - 59, Replace the force unwraps in makeSolidImage and pixelData with try `#require`, and mark both helpers as throws. Update every call site in the affected tests to use try, including the five references around lines 65, 75, 80, 134, and 140, and mark their enclosing test methods as throws.ThawTests/Utilities/ConcurrencyHelpersTests.swift (1)
77-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Poll for the cancellation instead of sleeping a fixed 200 ms.
The fixed sleep couples the assertion to scheduler latency. Under load on CI the losing arm can observe cancellation later than 200 ms, and the test fails intermittently. Poll until a deadline instead.
♻️ Proposed change
- // The cancellation is delivered after withTimeout has already thrown, - // so give the losing arm a moment to observe it. - try await Task.sleep(for: .milliseconds(200)) - `#expect`(observed.withLock { $0 }) + // The cancellation is delivered after withTimeout has already thrown, + // so wait for the losing arm to observe it. + let deadline = ContinuousClock.now + .seconds(5) + while !observed.withLock({ $0 }), ContinuousClock.now < deadline { + try await Task.sleep(for: .milliseconds(20)) + } + `#expect`(observed.withLock { $0 })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// The cancellation is delivered after withTimeout has already thrown, // so wait for the losing arm to observe it. let deadline = ContinuousClock.now + .seconds(5) while !observed.withLock({ $0 }), ContinuousClock.now < deadline { try await Task.sleep(for: .milliseconds(20)) } `#expect`(observed.withLock { $0 })🤖 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 `@ThawTests/Utilities/ConcurrencyHelpersTests.swift` around lines 77 - 80, Replace the fixed Task.sleep in the cancellation test with polling of observed until it becomes true or a reasonable deadline expires. Use the existing observed lock access, include a short interval between polls, and retain the final expectation so the test tolerates scheduler latency without waiting indefinitely.ThawTests/Utilities/IceSettingsImporterTests.swift (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Isolate this suite the same way
IceSettingsImporterTailTestsis isolated.
IceSettingsImporterwrites throughDefaults, andDefaults.storeis process-wide (Thaw/Utilities/Defaults.swift:127-129).ThawTests/Utilities/IceSettingsImporterTailTests.swift:24-27records this and therefore marks that suite.serializedand wraps every case inwithScratchDefaults. This suite does neither.The current fixtures happen to contain only an appearance key, so the import currently writes only through the injected closure. Any fixture that later adds a general, advanced, or hotkey key would write into the host application's real defaults domain and would race with other suites.
♻️ Proposed change
`@Suite`("Ice settings importer") `@MainActor` -struct IceSettingsImporterTests { +@Suite("Ice settings importer", .serialized) +struct IceSettingsImporterTests { `@Test`("A V1 appearance is converted and written as V2") func convertsV1Appearance() throws { - var oldConfiguration = MenuBarAppearanceConfigurationV1.defaultConfiguration + try withScratchDefaults { _ in + // existing body, indented one level + }🤖 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 `@ThawTests/Utilities/IceSettingsImporterTests.swift` around lines 13 - 15, Update IceSettingsImporterTests to match the isolation used by IceSettingsImporterTailTests: mark the suite as serialized and wrap every test case’s Defaults-backed work in withScratchDefaults. Ensure all IceSettingsImporter fixture imports execute against scratch defaults rather than the process-wide host defaults.ThawTests/Utilities/ShareableContentCacheTests.swift (1)
78-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Replace the sleep-based head starts with a causal signal.
Both concurrency cases establish caller ordering with a 20 ms sleep.
.serializedremoves contention from sibling cases in this suite. It does not remove contention from machine load on CI. If the second caller reachescontentbefore the first registers its in-flight task, each caller starts its own fetch and thecount == 1assertions on Lines 89 and 120 fail without a product defect.Make the ordering causal. Let the first fetch signal that it has begun, and let the test await that signal before starting the other callers. Then the head start no longer depends on elapsed time.
♻️ Sketch of a causal gate
+/// Signals that the shared fetch has started, so callers can join it +/// without racing a wall-clock head start. +private actor Gate { + private var continuations: [CheckedContinuation<Void, Never>] = [] + private var isOpen = false + + func open() { + isOpen = true + for continuation in continuations { continuation.resume() } + continuations.removeAll() + } + + func wait() async { + if isOpen { return } + await withCheckedContinuation { continuations.append($0) } + } +}`@Test`("Concurrent callers join the single in-flight fetch") func concurrentCallersJoinSingleInFlightFetch() async throws { let cache = ShareableContentCache<Int>() let invocationCount = Counter() + let started = Gate() `@Sendable` func slowFetch() async throws -> Int { await invocationCount.increment() + await started.open() try await Task.sleep(for: .milliseconds(150)) return 42 } async let first = cache.content(maxAge: .seconds(60), fetch: slowFetch) - // Give the first call a head start so it's the one that creates the - // in-flight task, then join it with two more concurrent callers. - try await Task.sleep(for: .milliseconds(20)) + // The first call has created the in-flight task; now join it. + await started.wait() async let second = cache.content(maxAge: .seconds(60), fetch: slowFetch) async let third = cache.content(maxAge: .seconds(60), fetch: slowFetch)Apply the same gate on Line 110 before
cancelledTask.cancel().Also applies to: 110-111
🤖 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 `@ThawTests/Utilities/ShareableContentCacheTests.swift` around lines 78 - 84, Replace the 20 ms sleeps in both concurrency cases with a causal synchronization gate: have the first fetch signal when it begins, then await that signal before starting the subsequent callers. Apply the same gate before cancelledTask.cancel() in the second case, preserving the existing fetch-count and cancellation assertions without relying on elapsed time.ThawTests/Utilities/SimpleSemaphoreTests.swift (1)
66-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The race hammer stops racing after the first iteration and inflates the permit count.
SimpleSemaphore.signal()increments a permit counter. Trace the loop with that in mind, starting fromvalue == 0after Line 66:
- Iteration 1:
releaseTasksignals (value == 1). The waiter acquires (value == 0) or times out (value == 1).- Acquired branch: Line 94 signals again, so
value == 1at the end of the iteration.- Timeout branch: no correction runs, so
value == 1at the end of the iteration.Every later iteration therefore starts with a permit already available.
wait(timeout: .milliseconds(1))then succeeds immediately and never races the release. The net change per iteration is+1(onesignalfromreleaseTask, onewait, one extrasignal), sovaluereaches roughly 500 by the end and the final check at Line 103 cannot fail.The two branches are inverted. When the waiter acquires, it holds the permit that
releaseTaskrestored, so nothing more is needed. When the waiter times out, the restored permit is unheld and must be consumed to return to the held state.🐛 Proposed fix
let acquired = await waitTask _ = await releaseTask - if acquired { - // The waiter won the race and now holds the permit that the - // release() call restored; give it back for the next - // iteration. - await semaphore.signal() - } - // If the waiter timed out, reconciliation inside wait(timeout:) - // already gave back any permit it might have raced into, and - // release() already restored the held permit, so the semaphore - // is back to "held by nobody, value == 1" either way. + // Restore the "held by exactly one caller" state the next + // iteration races against. + // + // If the waiter acquired, it already holds the permit that + // signal() restored, so there is nothing to do. If it timed out, + // reconciliation gave back any permit it raced into and the + // restored permit is now unheld, so take it back. + if !acquired { + try await semaphore.wait(timeout: .seconds(1)) + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try await semaphore.wait(timeout: .milliseconds(50)) for _ in 0 ..< 500 { async let releaseTask: Void = { let delay = Duration.nanoseconds(Int.random(in: 0 ... 2_000_000)) // 0-2 ms try? await Task.sleep(for: delay) await semaphore.signal() }() async let waitTask: Bool = { do { try await semaphore.wait(timeout: .milliseconds(1)) return true } catch is SimpleSemaphore.TimeoutError { return false } catch { Issue.record("Unexpected error: \(error)") return false } }() let acquired = await waitTask _ = await releaseTask // Restore the "held by exactly one caller" state the next // iteration races against. // // If the waiter acquired, it already holds the permit that // signal() restored, so there is nothing to do. If it timed out, // reconciliation gave back any permit it raced into and the // restored permit is now unheld, so take it back. if !acquired { try await semaphore.wait(timeout: .seconds(1)) } } // Final sanity check: the semaphore must be fully available. try await semaphore.wait(timeout: .seconds(1)) await semaphore.signal()🤖 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 `@ThawTests/Utilities/SimpleSemaphoreTests.swift` around lines 66 - 105, Correct the reconciliation logic in the semaphore race loop: in the acquired branch after awaiting waitTask, do not call semaphore.signal(), while in the timeout branch consume the restored permit with semaphore.wait(timeout:) so each iteration returns to the held state. Update the surrounding comments to match this behavior and preserve the final sanity check.ThawTests/Settings/Models/AutomationSettingsTests.swift (1)
13-45: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Three suites still snapshot the real
UserDefaultsdomain instead of usingwithScratchDefaults. All three predate the injectableDefaults.storethis PR adds, and each documents thatDefaultsis hardcoded to.standard. Each therefore mutates the developer's real preferences and restores them from aninit/deinitsnapshot..serializedorders only the cases inside one suite, so a parallel suite that touches the same keys can read the intermediate state or have its write discarded by the restore.ThawTests/Settings/Models/SettingsResetterTests.swiftlines 25-30 record this pattern as the cause of an intermittent cross-suite failure.
ThawTests/Settings/Models/AutomationSettingsTests.swift#L13-L45: delete thesavedDefaultssnapshot, theinitwipe, and thedeinitrestore; wrap each case body inwithScratchDefaults.ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift#L28-L55: delete the snapshot and restore; movemakeManagerand every assertion insidewithScratchDefaults, including the reload cases at lines 253-274.ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift#L17-L46: delete the snapshot and restore; wrap each case inwithScratchDefaultsand correct the doc comment that states the store is hardcoded.📍 Affects 3 files
ThawTests/Settings/Models/AutomationSettingsTests.swift#L13-L45(this comment)ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift#L28-L55ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift#L17-L46🤖 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 `@ThawTests/Settings/Models/AutomationSettingsTests.swift` around lines 13 - 45, Replace real UserDefaults snapshot/restore isolation with withScratchDefaults in ThawTests/Settings/Models/AutomationSettingsTests.swift (lines 13-45), removing savedDefaults, init wiping, and deinit restoration and wrapping each test case body. Apply the same change in ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift (lines 28-55), moving makeManager and all assertions, including reload cases, inside withScratchDefaults. In ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift (lines 17-46), remove snapshot/restore logic, wrap each case in withScratchDefaults, and update the doc comment to reflect the injectable store.ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift (1)
66-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect CI test invocation for the presence of a window server / display. set -euo pipefail fd --extension yml --extension yaml . .github/workflows --exec rg -n 'xcodebuild|test-without-building|runs-on|destination' {}Repository: thaw-app/Thaw
Length of output: 1384
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate files ==" fd -a 'DisplaySettingsManagerGlobalFallbackTests.swift|DisplaySettingsManagerMutationTests.swift|Bridging.*' . | sed 's#^\./##' echo echo "== target workflow xcodebuild lines ==" fd --extension yml --extension yaml . .github/workflows -x sh -c 'echo "--- $1"; rg -n -C 2 "xcodebuild test|-destination '\''platform=macOS'\''" "$1"' sh {} echo echo "== reference test snippets ==" for f in $(fd 'DisplaySettingsManagerGlobalFallbackTests.swift|DisplaySettingsManagerMutationTests.swift' .); do echo "--- $f" rg -n -C 3 "resolvedDisplayUsesExplicitConfiguration|resolvedDisplayWithoutOverrideFallsBackToGlobalConfiguration|activeDisplayWithoutOverrideFallsBackToGlobalConfiguration|activeDisplayUsesExplicitConfiguration|getDisplayUUIDString|getActiveMenuBarDisplayUUID|return early|guard" "$f" || true doneRepository: thaw-app/Thaw
Length of output: 1828
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== display bridge declarations/usages ==" rg -n "getDisplayUUIDString|getActiveMenuBarDisplayUUID|func getDisplayUUIDString|func getActiveMenuBarDisplayUUID" . --glob '!**/.build/**' --glob '!**/.git/**' echo echo "== Settings/Bridging related files ==" fd 'Bridging|Display.*Bridge|CG.*|Display.*' . --type f | sed 's#^\./##' | head -100Repository: thaw-app/Thaw
Length of output: 6303
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Bridging.swift display helpers ==" sed -n '108,152p' Shared/Bridging/Bridging.swift echo echo "== Global fallback test relevant lines ==" sed -n '60,102p' ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift echo echo "== Mutation test relevant lines ==" sed -n '138,152p' ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift echo echo "== Test workflow snippets around xcodebuild test ==" sed -n '100,160p' .github/workflows/ci.ymlRepository: thaw-app/Thaw
Length of output: 6893
Skip fallback-display tests when the display UUID cannot be resolved.
Bridging.getDisplayUUIDString(for:)andBridging.getActiveMenuBarDisplayUUID()returnnilwhen no display or no menu-bar display is available, sotry#require(...)turns an unavailable display into a test failure.DisplaySettingsManagerMutationTests.swiftguards the same lookups with early returns. Gate these cases with a trait or return early so runtimes without an attached display do not fail coverage.🤖 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 `@ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift` around lines 66 - 100, The fallback tests that depend on display identity should skip when UUID lookup is unavailable instead of failing via `#require`. Update resolvedDisplayWithoutOverrideFallsBackToGlobalConfiguration and activeDisplayUsesExplicitConfiguration to guard their respective Bridging lookups and return early (or apply the project’s existing skip trait), matching the early-return pattern in DisplaySettingsManagerMutationTests; leave tests with valid UUIDs unchanged.
diazdesandi
added a commit
that referenced
this pull request
Jul 31, 2026
Coverage came back 0% on PR #861 with every one of 5,606 lines reported uncovered -- the analysis matched no coverage data at all. xcresultparser emits absolute paths from the macOS runner (/Users/runner/work/Thaw/Thaw/...), and the Sonar scan runs on a separate Linux runner that matches coverage entries to sources by path. Every entry therefore failed to resolve. scripts/normalize-coverage.py exists to rewrite those to repo-relative and had simply stopped being called; the script was still in the tree, just orphaned. Verified against the actual artifact from run 30652941215 rather than in principle: of 389 file entries, the script keeps 309 repo-relative and drops 80 build artifacts, leaving zero absolute paths. This is the same regression sonar-project.properties documented once before, when the report path moved into the workflow and the normalize step went with it. The gated `-Dsonar.coverageReportPaths` arg is deliberate and stays as it is -- docs-only scans would otherwise point at a file the build never produced. Only the normalize step was missing. The script exits non-zero if no entry resolves, so a future break fails the job instead of silently publishing 0% again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (1)
5169-5208: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSort items before selecting return anchors.
itemsis in Window Server order, not physical menu-bar order.firstIndex,eligibleIndices, andLayoutSolver.returnAnchorstherefore select sequence neighbors that can be unrelated to the item's actual left and right neighbors.This can return an item beside an arbitrary same-section item. The resulting order can persist after relaunch. Sort by leading edge before deriving indices and destinations.
Proposed fix
private func getReturnDestination( for item: MenuBarItem, in items: [MenuBarItem], section: MenuBarSection.Name ) -> (destination: MoveDestination, fallbackNeighbor: (tag: MenuBarItemTag, pid: pid_t)?)? { - guard let index = items.firstIndex(matching: item.tag) else { + let orderedItems = MenuBarItem.sortByLeadingEdgeThenIdentifier(items) + guard let index = orderedItems.firstIndex(matching: item.tag) else { return nil } - let eligibleIndices = Set(items.indices.filter { candidateIndex in - let candidate = items[candidateIndex] + let eligibleIndices = Set(orderedItems.indices.filter { candidateIndex in + let candidate = orderedItems[candidateIndex] guard candidate.canBeHidden else { return false } return itemCache.address(for: candidate.tag)?.section == section }) let anchors = LayoutSolver.returnAnchors( forIndex: index, - itemCount: items.count, + itemCount: orderedItems.count, eligibleIndices: eligibleIndices ) if let successor = anchors.successor { let fallback = anchors.predecessor.map { predecessor in - let neighbor = items[predecessor] + let neighbor = orderedItems[predecessor] return (neighbor.tag, neighbor.sourcePID ?? neighbor.ownerPID) } - return (.leftOfItem(items[successor]), fallback) + return (.leftOfItem(orderedItems[successor]), fallback) } if let predecessor = anchors.predecessor { - return (.rightOfItem(items[predecessor]), nil) + return (.rightOfItem(orderedItems[predecessor]), nil) } - return sectionDestination(for: section, in: items).map { ($0, nil) } + return sectionDestination(for: section, in: orderedItems).map { ($0, nil) } }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 5169 - 5208, Update getReturnDestination to derive anchor indices from items ordered by each item’s physical leading edge rather than Window Server order. Sort the items before firstIndex, eligibleIndices, LayoutSolver.returnAnchors, and destination selection, while preserving the existing section filtering and fallback behavior.
🤖 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.
Outside diff comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 5169-5208: Update getReturnDestination to derive anchor indices
from items ordered by each item’s physical leading edge rather than Window
Server order. Sort the items before firstIndex, eligibleIndices,
LayoutSolver.returnAnchors, and destination selection, while preserving the
existing section filtering and fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e90ee32-88ec-430d-a96d-c92d771b58af
📒 Files selected for processing (6)
.github/workflows/ci.ymlMenuBarItemService/SourcePIDCache.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftscripts/swiftlint-inputs.xcfilelistsonar-project.properties
diazdesandi
added a commit
that referenced
this pull request
Aug 1, 2026
Coverage came back 0% on PR #861 with every one of 5,606 lines reported uncovered -- the analysis matched no coverage data at all. xcresultparser emits absolute paths from the macOS runner (/Users/runner/work/Thaw/Thaw/...), and the Sonar scan runs on a separate Linux runner that matches coverage entries to sources by path. Every entry therefore failed to resolve. scripts/normalize-coverage.py exists to rewrite those to repo-relative and had simply stopped being called; the script was still in the tree, just orphaned. Verified against the actual artifact from run 30652941215 rather than in principle: of 389 file entries, the script keeps 309 repo-relative and drops 80 build artifacts, leaving zero absolute paths. This is the same regression sonar-project.properties documented once before, when the report path moved into the workflow and the normalize step went with it. The gated `-Dsonar.coverageReportPaths` arg is deliberate and stays as it is -- docs-only scans would otherwise point at a file the build never produced. Only the normalize step was missing. The script exits non-zero if no entry resolves, so a future break fails the job instead of silently publishing 0% again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
diazdesandi
force-pushed
the
test/swift-testing-migration-and-coverage
branch
from
August 1, 2026 00:17
4155e4f to
652500f
Compare
Coverage came back 0% on PR #861 with every one of 5,606 lines reported uncovered -- the analysis matched no coverage data at all. xcresultparser emits absolute paths from the macOS runner (/Users/runner/work/Thaw/Thaw/...), and the Sonar scan runs on a separate Linux runner that matches coverage entries to sources by path. Every entry therefore failed to resolve. scripts/normalize-coverage.py exists to rewrite those to repo-relative and had simply stopped being called; the script was still in the tree, just orphaned. Verified against the actual artifact from run 30652941215 rather than in principle: of 389 file entries, the script keeps 309 repo-relative and drops 80 build artifacts, leaving zero absolute paths. This is the same regression sonar-project.properties documented once before, when the report path moved into the workflow and the normalize step went with it. The gated `-Dsonar.coverageReportPaths` arg is deliberate and stays as it is -- docs-only scans would otherwise point at a file the build never produced. Only the normalize step was missing. The script exits non-zero if no entry resolves, so a future break fails the job instead of silently publishing 0% again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
- Roll back in-memory profile state when a profile apply is cancelled mid-flight, guarded by an apply token so a late-arriving cancellation cannot clobber the state armed by the newer apply that displaced it; the cancelled apply also skips the deferred cache refresh. - Replace the duplicated "FocusFilterRequestedProfileID" literals with a shared Defaults.Key (raw value unchanged). - Extract validatedDisplayConfiguration(forUUID:) in SettingsURIHandler and use it from both per-display set and toggle paths. - Move the misplaced doc comment off diagLog so it precedes getMenuBarItemWindows, and drop the inaccurate duplicate before assignStableInstanceIndices(to:using:). - Add CaseIterable to DiagnosticLogger.Level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
- Serialize withScratchDefaults through a FIFO actor-backed mutex and document the non-recursive sync lock; migrate the Automation, DisplaySettingsManager, Hotkeys, AdvancedSettings, IceSettingsImporter, ProfileManagerUpdateRearm, and SettingsURIHandler Apply/Get/Tail suites off real-UserDefaults snapshot/restore onto the scratch store. - Extract shared fixtures: ProfileTestFixtures (makeProfile, seedManifest, snapshot/content factories, temp-dir and codec helpers) and GraphicsTestFixtures (premultiplied-first 32-bit-LE makeCanvas, makeOpaqueImage), removing the per-suite duplicates. - Replace tautological assertions with real ones: localized section names, parsed URI routes, registered-default comparisons, sRGB color space name, menu-agent auto-detected source name. - Harden flaky-prone tests: .timeLimit traits on the permission-polling and empty-profile-apply tests, skip-instead-of-fail for display- and window-server-dependent tests, polling and causal gates in place of sleeps in the concurrency/cache tests, corrected semaphore permit accounting, try #require in place of force unwraps, non-empty guards before iterating stored configurations, and snapshot/restore of the unresponsive-items build key in the failure-ledger suite. - Cover the alwaysHiddenSectionResolved and hiddenSectionHasRoom gates in shouldPersistSavedOrder; drop the three case-existence LayoutResetError tests; rename the parser's nested Set suite to SetRoute; keep droppyURIs the single source of truth for Droppy URL shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
…port Replace the hand-rolled option loop with argparse (invalid --top now produces a usage error), and make dir/*.swift exclusion patterns match only direct children while recursive ** patterns still go through fnmatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
The PR's new-code coverage sat just under the 80% gate, almost entirely because performSetup(with:) — which needs a live AppState and is therefore untestable by convention — contained three copies of the observer -> AsyncStream -> debounce wiring. Extract the wiring into a shared debouncedNotificationTask helper in ConcurrencyHelpers (observer registered before the task starts, removed by its defer) and move the three call sites into an internal startObservationTasks() that unit tests can drive without an AppState. New tests cover burst coalescing, repeated bursts, cancellation, and the re-setup path cancelling the previous tasks. Lifts new-code coverage of measured files from ~73% to ~92% locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
getReturnDestination fed LayoutSolver.returnAnchors indices from the item list as enumerated, which is Window Server order — so the "successor" anchor could be a physically distant item and returning a temporarily shown item could scramble its position. Sort the items by leading edge before deriving the index, eligible set, anchors, and destination; section filtering and the section-boundary fallback are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: diazdesandi <diazdesandi@proton.me>
diazdesandi
force-pushed
the
test/swift-testing-migration-and-coverage
branch
from
August 1, 2026 00:23
652500f to
f3d078b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
ThawTests/Settings/Models/ExternalSettingsChangeTests.swift (1)
49-50: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the asynchronous wait.
If the observer does not receive the valid notification,
withCheckedContinuationdoes not resume. The test then hangs instead of failing. Add.timeLimit(.minutes(1))to the@Teston Line 49.Proposed fix
- `@Test`("The observer ignores malformed notifications and delivers valid changes") + `@Test`( + "The observer ignores malformed notifications and delivers valid changes", + .timeLimit(.minutes(1)) + )🤖 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 `@ThawTests/Settings/Models/ExternalSettingsChangeTests.swift` around lines 49 - 50, Add the .timeLimit(.minutes(1)) trait to the `@Test` annotation for observerFiltersAndDelivers so the asynchronous test fails after a bounded wait instead of hanging when the continuation is not resumed.ThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swift (1)
29-53: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRoute this suite through
withScratchDefaultsinstead of save-and-restore.The
.unresponsiveMenuBarItemsBuildsnapshot closes the gap raised earlier. The suite still writes to the realDefaultsstore and depends oninit/deinitto put the user's values back.Two problems remain with that approach:
- A crash or a forced test-run stop leaves the developer's ledger keys in the state a test left them.
- Another suite can install a scratch store through
withScratchDefaultswhile this suite runs, because.serializedorders tests inside one suite only. The reads at Lines 94, 164, 176, and 232 then observe someone else's store.
ThawTests/Support/ScratchDefaults.swiftdocuments the intended target state: everyDefaults-touching suite routes through the helper. Wrapping each test body removes both the snapshot fields and the interference window.🤖 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 `@ThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swift` around lines 29 - 53, Refactor MenuBarItemFailureLedgerTests to run every Defaults-touching test body through withScratchDefaults, including the reads around the referenced test sections. Remove the savedValue/savedBuildValue properties and init/deinit save-and-restore logic, and ensure each test uses the isolated scratch store for its entire execution.ThawTests/Settings/Models/AdvancedSettingsTests.swift (1)
62-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated notification-posting helper into a shared test fixture.
postExternalChanges/postExternalChangeare implemented identically, with identical documentation, in both suites. The shared root cause is a missing shared test fixture for "post toNotificationCenter.defaultand wait for main-queue delivery."
ThawTests/Settings/Models/AdvancedSettingsTests.swift#L62-L80: remove the localpostExternalChanges/postExternalChangepair and call a shared helper instead.ThawTests/Settings/Models/GeneralSettingsTests.swift#L111-L129: remove the localpostExternalChanges/postExternalChangepair and call the same shared helper.Move the pair into a shared file under
ThawTests/Support/(for example alongsideThawTests/Support/ScratchDefaults.swift), the same treatment already applied to the duplicated bitmap-context builder inThawTests/Support/GraphicsTestFixtures.swift.🤖 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 `@ThawTests/Settings/Models/AdvancedSettingsTests.swift` around lines 62 - 80, The notification-posting helpers are duplicated across both settings test suites. Create a shared test fixture under ThawTests/Support containing postExternalChanges and postExternalChange, preserving NotificationCenter.default posting and main-queue delivery waiting; remove both local helper pairs from ThawTests/Settings/Models/AdvancedSettingsTests.swift lines 62-80 and ThawTests/Settings/Models/GeneralSettingsTests.swift lines 111-129, and update each suite to call the shared helpers.ThawTests/Shared/WindowInfoTests.swift (1)
18-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the fixture JSON with
JSONSerializationinstead of string interpolation.Lines 28-29 interpolate
titleandownerNameinto quoted JSON string literals without escaping. A caller that passes a title containing a double quote or a backslash produces invalid JSON. The test then fails with aDecodingErrorinstead of a clear assertion failure, and the cause is not obvious from the message.Current callers pass only simple identifiers, so nothing fails today.
WindowInfo.titlecarries arbitrary window titles in production, so a test for a quoted title is a realistic addition. Constructing the object graph and serializing it removes the escaping concern and shortens the helper.♻️ Proposed refactor
) throws -> WindowInfo { - // CGRect encodes as nested arrays: [[x,y],[width,height]] - let titleJSON = title.map { "\"\($0)\"" } ?? "null" - let ownerNameJSON = ownerName.map { "\"\($0)\"" } ?? "null" - let json = """ - { - "windowID": \(windowID), - "ownerPID": \(ownerPID), - "bounds": [[\(bounds.origin.x), \(bounds.origin.y)], [\(bounds.size.width), \(bounds.size.height)]], - "layer": \(layer), - "title": \(titleJSON), - "ownerName": \(ownerNameJSON), - "isOnScreen": \(isOnScreen) - } - """ - let data = Data(json.utf8) + // CGRect encodes as nested arrays: [[x,y],[width,height]] + let object: [String: Any] = [ + "windowID": windowID, + "ownerPID": ownerPID, + "bounds": [ + [bounds.origin.x, bounds.origin.y], + [bounds.size.width, bounds.size.height], + ], + "layer": layer, + "title": title ?? NSNull(), + "ownerName": ownerName ?? NSNull(), + "isOnScreen": isOnScreen, + ] + let data = try JSONSerialization.data(withJSONObject: object) return try JSONDecoder().decode(WindowInfo.self, from: data) }🤖 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 `@ThawTests/Shared/WindowInfoTests.swift` around lines 18 - 43, Update createWindowInfo to construct the fixture as a Swift object graph and serialize it with JSONSerialization instead of interpolating title and ownerName into JSON text. Preserve the existing field values, including optional nil handling and nested bounds structure, so titles containing quotes or backslashes produce valid JSON for WindowInfo decoding.Thaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swift (1)
132-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated initializer call and assert the parallel-array assumption.
Both branches at Lines 135-147 call the same initializer with the same arguments. In the
elsebranch,nilis exactlyitems[itemIndex].sourcePID. The branch adds no behavior.The function also depends on an unstated contract:
items[i]must describewindows[i]. Both current callers satisfy it, because each buildsitemsas an order-preserving 1:1 map ofwindows. A future caller that filtersitemswould either rebuild an item from the wrong window or trap onwindows[itemIndex]. Add apreconditionso that contract fails loudly instead of silently mismatching.♻️ Proposed refactor
+ precondition( + items.count == windows.count, + "assignStableInstanceIndices requires items to be a 1:1, order-preserving map of windows" + ) var groups = [String: [Int]]() for i in 0 ..< items.count { let key = "\(items[i].tag.namespace):\(items[i].tag.title)" groups[key, default: []].append(i) } for (_, indices) in groups where indices.count > 1 { let sorted = indices.sorted { items[$0].windowID < items[$1].windowID } for (instanceIndex, itemIndex) in sorted.enumerated() where instanceIndex > 0 { - if let sourcePID = items[itemIndex].sourcePID { - items[itemIndex] = MenuBarItem( - uncheckedItemWindow: windows[itemIndex], - sourcePID: sourcePID, - instanceIndex: instanceIndex - ) - } else { - items[itemIndex] = MenuBarItem( - uncheckedItemWindow: windows[itemIndex], - sourcePID: nil, - instanceIndex: instanceIndex - ) - } + items[itemIndex] = MenuBarItem( + uncheckedItemWindow: windows[itemIndex], + sourcePID: items[itemIndex].sourcePID, + instanceIndex: instanceIndex + ) } }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift around lines 132 - 148, In the enumeration function containing the grouped-item loop, add a precondition establishing that the parallel items and windows collections have equal counts before indexing them together. Then simplify the instance-rebuild loop to one MenuBarItem initializer using the existing items[itemIndex].sourcePID value directly, removing the redundant if/else branches while preserving source PID and instance index behavior.Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (1)
7024-7030: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate the no-move exits for profile applies before clearing the snapshot.
persistProfileStateOnSuccessclearspriorProfileApplySnapshotfor every.profilesuccess path. Before its callers that can be reached by a cancelled task use it, check whether that task still owns the currentprofileApplyTokenand roll back throughrestoreProfileStateAfterCancelledApplyif a newer apply has already started.Apply the same guard to the
itemOrder.isEmpty,fullSequence.isEmpty, andplannedMoves.isEmptyexits.🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 7024 - 7030, Guard every profile-apply no-move exit for itemOrder.isEmpty, fullSequence.isEmpty, and plannedMoves.isEmpty before calling persistProfileStateOnSuccess. Verify the task still owns the current profileApplyToken; if ownership is lost, call restoreProfileStateAfterCancelledApply instead of clearing priorProfileApplySnapshot, otherwise continue the existing success persistence path.
♻️ Duplicate comments (1)
ThawTests/Support/ScratchDefaults.swift (1)
113-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe async scratch-defaults lane still does not exclude the synchronous lane.
ScratchDefaultsMutexorders async callers only, so a synchronous suite can install and delete a scratch suite between the async lane's read ofDefaults.storeand its restore. The async lane then republishes a deleted store as the process-wideDefaults.store. The suite documentation that depends on this guarantee is wrong for the same reason.
ThawTests/Support/ScratchDefaults.swift#L113-L143: takescratchDefaultsLockacross the non-suspending store swap and across the restore in thedefer, then narrow the warning to the non-recursive constraint.ThawTests/Settings/Models/FocusFilterIntentTests.swift#L32-L34: state that the async overload serializes against other async callers only, or drop the serialization claim once the helper covers both lanes.🤖 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 `@ThawTests/Support/ScratchDefaults.swift` around lines 113 - 143, Update ThawTests/Support/ScratchDefaults.swift: in withScratchDefaults, hold scratchDefaultsLock across the synchronous Defaults.store swap and defer-based restore, while retaining ScratchDefaultsMutex for the full async body; narrow the warning to the mutex’s non-recursive constraint. Update ThawTests/Settings/Models/FocusFilterIntentTests.swift:32-34 to state that the async overload serializes only with other async callers, or remove that serialization claim if the helper now covers both lanes.
🤖 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 `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 170-188: Extract the Focus Filter notification names into shared
Notification.Name constants, alongside the existing shared Focus Filter defaults
key, so both targets use one definition. Update the consumer in the
focusFilterActivatedTask and focusFilterDeactivatedTask setup to reference those
constants instead of string literals, and update the ThawFocusFilter producer to
post the same shared constants while preserving the existing notification
values.
In `@ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift`:
- Around line 182-194: Update globalBroadcastWithNoDisplays to assert the
intended no-target contract under a controlled setup, ensuring connected
displays cannot contribute targets; alternatively rename the test to describe
the configuration-application invariant it currently verifies. Keep the
assertion meaningful and avoid relying on a potentially empty loop.
In `@ThawTests/Settings/Models/FocusFilterIntentTests.swift`:
- Around line 32-34: Update the suite documentation comment describing the async
withScratchDefaults overload to state that it serializes only against other
async callers, not the entire process or synchronous callers. Leave the
.serialized rationale about nonisolated static variables unchanged.
In `@ThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swift`:
- Around line 97-103: Update everyBooleanKeyIsSettable to always use
.settingsDidChangeViaURI instead of selecting a channel via
perDisplayKeys.contains(key). Remove the now-unreachable
.perDisplaySettingsDidChangeViaURI branch while preserving the existing
boolean-key validation.
---
Outside diff comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift:
- Around line 132-148: In the enumeration function containing the grouped-item
loop, add a precondition establishing that the parallel items and windows
collections have equal counts before indexing them together. Then simplify the
instance-rebuild loop to one MenuBarItem initializer using the existing
items[itemIndex].sourcePID value directly, removing the redundant if/else
branches while preserving source PID and instance index behavior.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 7024-7030: Guard every profile-apply no-move exit for
itemOrder.isEmpty, fullSequence.isEmpty, and plannedMoves.isEmpty before calling
persistProfileStateOnSuccess. Verify the task still owns the current
profileApplyToken; if ownership is lost, call
restoreProfileStateAfterCancelledApply instead of clearing
priorProfileApplySnapshot, otherwise continue the existing success persistence
path.
In `@ThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swift`:
- Around line 29-53: Refactor MenuBarItemFailureLedgerTests to run every
Defaults-touching test body through withScratchDefaults, including the reads
around the referenced test sections. Remove the savedValue/savedBuildValue
properties and init/deinit save-and-restore logic, and ensure each test uses the
isolated scratch store for its entire execution.
In `@ThawTests/Settings/Models/AdvancedSettingsTests.swift`:
- Around line 62-80: The notification-posting helpers are duplicated across both
settings test suites. Create a shared test fixture under ThawTests/Support
containing postExternalChanges and postExternalChange, preserving
NotificationCenter.default posting and main-queue delivery waiting; remove both
local helper pairs from ThawTests/Settings/Models/AdvancedSettingsTests.swift
lines 62-80 and ThawTests/Settings/Models/GeneralSettingsTests.swift lines
111-129, and update each suite to call the shared helpers.
In `@ThawTests/Settings/Models/ExternalSettingsChangeTests.swift`:
- Around line 49-50: Add the .timeLimit(.minutes(1)) trait to the `@Test`
annotation for observerFiltersAndDelivers so the asynchronous test fails after a
bounded wait instead of hanging when the continuation is not resumed.
In `@ThawTests/Shared/WindowInfoTests.swift`:
- Around line 18-43: Update createWindowInfo to construct the fixture as a Swift
object graph and serialize it with JSONSerialization instead of interpolating
title and ownerName into JSON text. Preserve the existing field values,
including optional nil handling and nested bounds structure, so titles
containing quotes or backslashes produce valid JSON for WindowInfo decoding.
---
Duplicate comments:
In `@ThawTests/Support/ScratchDefaults.swift`:
- Around line 113-143: Update ThawTests/Support/ScratchDefaults.swift: in
withScratchDefaults, hold scratchDefaultsLock across the synchronous
Defaults.store swap and defer-based restore, while retaining
ScratchDefaultsMutex for the full async body; narrow the warning to the mutex’s
non-recursive constraint. Update
ThawTests/Settings/Models/FocusFilterIntentTests.swift:32-34 to state that the
async overload serializes only with other async callers, or remove that
serialization claim if the helper now covers both lanes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e439667e-f37a-4a46-a607-47fcebe6cb88
📒 Files selected for processing (55)
Shared/Utilities/DiagnosticLogger.swiftThaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/Settings/Models/FocusFilterIntent.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Utilities/ConcurrencyHelpers.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/SettingsURIHandler.swiftThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swiftThawTests/MenuBar/Items/MenuBarItemValueTests.swiftThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swiftThawTests/MenuBar/Layout/LayoutResetErrorTests.swiftThawTests/MenuBar/Section/MenuBarSectionNameTests.swiftThawTests/Permissions/PermissionTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/AutomationSettingsTests.swiftThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swiftThawTests/Settings/Models/ExternalSettingsChangeTests.swiftThawTests/Settings/Models/FocusFilterIntentTests.swiftThawTests/Settings/Models/GeneralSettingsSnapshotTests.swiftThawTests/Settings/Models/GeneralSettingsTests.swiftThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swiftThawTests/Settings/Models/ProfileApplyInFlightFlagTests.swiftThawTests/Settings/Models/ProfileExportBundleTests.swiftThawTests/Settings/Models/ProfileManagerCRUDTests.swiftThawTests/Settings/Models/ProfileManagerDeepTests.swiftThawTests/Settings/Models/ProfileManagerDeleteTests.swiftThawTests/Settings/Models/ProfileManagerObservationTasksTests.swiftThawTests/Settings/Models/ProfileManagerPersistenceTests.swiftThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swiftThawTests/Settings/Models/ProfileTests.swiftThawTests/Settings/Search/SearchModelTests.swiftThawTests/Settings/URI/IntegrationURIContractTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Settings/URI/SettingsURIHandlerGetTests.swiftThawTests/Settings/URI/SettingsURIHandlerTailTests.swiftThawTests/Settings/URI/SettingsURIParserTests.swiftThawTests/Shared/DiagnosticLoggerTests.swiftThawTests/Shared/WindowInfoTests.swiftThawTests/Support/GraphicsTestFixtures.swiftThawTests/Support/ProfileTestFixtures.swiftThawTests/Support/ScratchDefaults.swiftThawTests/Utilities/CGImageAnalysisTests.swiftThawTests/Utilities/CGImageDetachedCopyTests.swiftThawTests/Utilities/ConcurrencyHelpersTests.swiftThawTests/Utilities/ExtensionsGraphicsTests.swiftThawTests/Utilities/ExtensionsRemainderTests.swiftThawTests/Utilities/IceSettingsImporterTests.swiftThawTests/Utilities/MigrationTests.swiftThawTests/Utilities/ObjectStorageTests.swiftThawTests/Utilities/ShareableContentCacheTests.swiftThawTests/Utilities/SimpleSemaphoreTests.swiftscripts/coverage-report.py
💤 Files with no reviewable changes (6)
- ThawTests/Utilities/ExtensionsRemainderTests.swift
- ThawTests/MenuBar/Layout/LayoutResetErrorTests.swift
- ThawTests/Settings/Models/ProfileManagerCRUDTests.swift
- ThawTests/Settings/Models/ProfileManagerDeleteTests.swift
- ThawTests/Settings/Models/ProfileManagerDeepTests.swift
- ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (2)
202-207: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the enrichment gate read
Defaults.storeon each access.
static nonisolated letinitializes once, on first access, and caches the value for the process lifetime.Defaults.storeis now injectable, so the cached value depends on which store happens to be installed when some test first touches this property. A test that installs a scratch store afterwards reads the stale flag, and a test that touches the property while a scratch store is installed bakes that scratch value in for every later test in the same process.The flag only gates a diagnostic pass, so re-reading it per access costs one
UserDefaultslookup per cache cycle.🛡️ Proposed fix
- static nonisolated let isDegradedIdentityEnrichmentEnabled = - Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") + static nonisolated var isDegradedIdentityEnrichmentEnabled: Bool { + Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") + }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 202 - 207, Change isDegradedIdentityEnrichmentEnabled from a cached static let to a computed static property that reads Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") on every access, preserving the existing key and nonisolated behavior.
7051-7057: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAbort paths leave the never-committed profile spec armed in memory.
armProfileStateoverwritespinnedHiddenBundleIDs,pinnedAlwaysHiddenBundleIDs,savedSectionOrder,activeProfileLayout, andactiveProfileItemIdentifiers, and defers disk persistence topersistProfileStateOnSuccess. The newrestoreProfileStateAfterCancelledApplyundoes that overwrite, but only Phase 7 cancellation calls it (Line 8088).Four post-arm aborts call
clearProfileStateinstead, which clearsisApplyingProfileLayoutand nothing else:
- Line 7222: unresolved-sourcePID majority.
- Line 7231: a menu bar item menu is open.
- Line 7243: missing control items.
- Line 7955: control items lost after Phase 1.
No moves ran and nothing was persisted on any of these paths, so memory now describes a profile that disk does not hold. That is the exact condition the snapshot doc comment at Lines 495-500 names: the late-arrival re-sort path keeps sorting toward a profile that never committed.
priorProfileApplySnapshotalso stays retained until the next arm replaces it.Roll back on these aborts as well, using the same token guard.
🐛 Proposed fix
/// Profile-only exit cleanup: refresh the sorted snapshot and clear /// the in-flight profile flag. No-op for .savedOrder. private func clearProfileState(source: ApplySource, items: [MenuBarItem]) { updateProfileSortedSnapshot(source: source, items: items) guard case .profile = source else { return } isApplyingProfileLayout = false } + + /// Exit cleanup for a profile apply that aborted after arming without + /// moving anything and without persisting. Reverts the armed spec so + /// memory matches what disk still holds, then runs the normal teardown. + private func abandonProfileApply( + source: ApplySource, + items: [MenuBarItem], + token: Int + ) { + if case .profile = source { + restoreProfileStateAfterCancelledApply(token: token) + } + clearProfileState(source: source, items: items) + }Then replace the four abort calls, for example at Line 7231:
if await isAnyMenuBarItemMenuOpen() { MenuBarItemManager.diagLog.info("applyProfileLayout: skipping, a menu bar item menu is open") - clearProfileState(source: source, items: items) + abandonProfileApply(source: source, items: items, token: applyToken) return }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 7051 - 7057, Update the post-arm abort handling in the functions containing the unresolved-sourcePID, open-menu, missing-control-item, and post-Phase-1 control-loss paths to call restoreProfileStateAfterCancelledApply with the same apply token guard instead of clearProfileState. Preserve clearProfileState for successful completion, and ensure each cancellation restores all armed profile state and releases priorProfileApplySnapshot without persisting changes.Thaw/Events/HIDEventManager.swift (1)
1085-1091: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the documented
try? awaitorder for the sleep guard.Swift requires
trybeforeawaitin expressions that use both operators.guard await (try? Task.sleep(...))violates that order; useguard (try? await Task.sleep(...))so the line is guaranteed to compile and matches the existingtry await Task.sleep(...)usage pattern.🤖 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 `@Thaw/Events/HIDEventManager.swift` around lines 1085 - 1091, Update the sleep guard inside the Task closure in HIDEventManager to use the Swift-required operator order, placing try? before await: guard (try? await Task.sleep(...)) != nil. Preserve the existing cancellation behavior and 250-millisecond delay.Thaw/Settings/Models/AdvancedSettings.swift (1)
270-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCorrect the doc comment: this function never returns the default order.
The doc states "Returns the default order if the input is unusable." The implementation does not do that. When
rawValuesis empty or holds only unrecognized values,preferredis empty and the result isMenuBarSection.Name.allCases, which is the declaration order rather thanDefaults.DefaultValue.searchSectionOrder.
SettingsResetterTests.resetAdvancedRestoresSearchSectionOrderderives its expectation by calling this same function, so it cannot detect the difference. Either update the doc to describe theallCasesfallback, or make the empty-input case return the sanitized default order.📝 Proposed doc correction
/// Returns a search-section order that contains each `MenuBarSection.Name` /// case exactly once, using the supplied raw values as the preferred order - /// and filling any missing cases at the end. Returns the default order if - /// the input is unusable. + /// and filling any missing cases at the end in `allCases` order. Unusable + /// raw values are dropped, so an input with no recognized value yields + /// `MenuBarSection.Name.allCases`. static func sanitizedSearchSectionOrder(from rawValues: [String]) -> [MenuBarSection.Name] {🤖 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 `@Thaw/Settings/Models/AdvancedSettings.swift` around lines 270 - 279, Correct the documentation for sanitizedSearchSectionOrder(from:) to state that empty or entirely unrecognized input falls back to MenuBarSection.Name.allCases in declaration order, not Defaults.DefaultValue.searchSectionOrder. Keep the implementation unchanged unless intentionally aligning its behavior with the documented default.
🤖 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 `@scripts/coverage-report.py`:
- Around line 125-151: Update the explanatory comment above the `best` map and
per-file deduplication loop to state that the implementation approximates Sonar
by retaining the entry with the greatest `coveredLines`, rather than unioning
covered or executable line sets across targets. Preserve the existing
deduplication logic.
In `@sonar-project.properties`:
- Line 85: Remove the whole-file coverage exclusion for AXItemActivator.swift
unless it is genuinely entirely AX-driven; preserve coverage for the pure logic
exercised by AXItemActivatorTests. If the file cannot be split, document the
specific AX-only rationale in the exclusion comment block above, consistent with
the existing whole-file exclusions.
In `@Thaw/Main/Updates.swift`:
- Around line 58-62: Update the allowsBetaUpdates property getter and setter to
use the centralized typed Defaults interface: replace the raw
"AllowsBetaUpdates" accesses with Defaults.bool(forKey: .allowsBetaUpdates) and
Defaults.set(_:forKey: .allowsBetaUpdates), including the additional occurrence
near the related update settings code.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift:
- Around line 132-149: In the instance-index rebuild loop within the enumeration
logic, replace the duplicated sourcePID branches with one MenuBarItem
initializer that passes items[itemIndex].sourcePID directly, while preserving
the existing uncheckedItemWindow and instanceIndex arguments.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 5207-5215: Update the orderedItems sort in the neighbor-resolution
flow to use windowID as a deterministic secondary key whenever bounds.minX
values are equal, producing a total reproducible order while preserving
leading-edge ordering as the primary key.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemTag.swift`:
- Line 9: Confirm that the package configuration declares the swift-algorithms
dependency required by the Algorithms import in MenuBarItemTag. Ensure the
target containing MenuBarItemTag links that product, distinguishing it from
swift-async-algorithms, and preserve the order-preserving deduplication
implementation.
In `@Thaw/MenuBar/Search/MenuBarSearchPanel.swift`:
- Around line 401-402: Remove the deprecated Defaults.store.synchronize() call
from the panel frame save path, while preserving the existing
Defaults.store.set(...) frame persistence.
In `@Thaw/Settings/Models/Profile.swift`:
- Around line 334-347: Update resolvedItemSectionMap to treat a non-nil empty
itemSectionMap as absent and fall back to the saved section map, matching
resolvedItemOrder. Also update captureCurrentLayout so empty derived itemOrder
and itemSectionMap collections are not persisted over existing valid saved
values.
- Around line 71-76: Update the snapshot-application logic around the
lastCustomIceIcon assignment in Profile so a nil snapshot value does not
overwrite the value populated by the .custom iceIcon didSet. Apply
lastCustomIceIcon only when the snapshot contains a value, while preserving the
reordered assignment for snapshots that provide one.
In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 652-660: Update ProfileManager.captureCurrentLayout() to read
savedSectionOrder, pinnedHiddenBundleIDs, and pinnedAlwaysHiddenBundleIDs using
their typed Defaults.Key cases from MenuBarItemManager instead of raw string
literals. Ensure the corresponding MenuBarItemManager writers and these readers
share the same compile-time key definitions.
In `@ThawTests/MenuBar/Items/CapturedImageScaleTests.swift`:
- Around line 118-135: Update resolvedScaleRestoresPointSize() to declare
throws, then replace try? `#require`(scale) with direct try `#require`(scale) so a
nil resolved scale fails with the framework’s clear requirement error instead of
falling through to the fallback arithmetic.
In `@ThawTests/MenuBar/Items/MenuBarTestFixturesTests.swift`:
- Around line 20-26: Update appItemTagBuildsExpectedNamespaceAndTitle to assert
tag.namespace directly against the expected Namespace case/value instead of
comparing String(describing: tag.namespace). Preserve the existing assertions
for title, instanceIndex, and windowID.
In `@ThawTests/MenuBar/Section/MenuBarSectionNameTests.swift`:
- Around line 90-111: Update localizedVisible, localizedHidden, and
localizedAlwaysHidden to compare each returned LocalizedStringKey directly with
the corresponding LocalizedStringKey literal using Equatable. Remove the
misleading String(describing:) comment and comparisons, while preserving the
existing expected keys.
In `@ThawTests/Settings/Models/AdvancedSettingsTests.swift`:
- Around line 46-80: Update AdvancedSettings to accept an injectable
NotificationCenter, matching the existing center injection pattern used by
debouncedNotificationTask, and use that center when observing settings changes.
In AdvancedSettingsTests, create a private NotificationCenter per test and pass
it to the model, then post through the same isolated center instead of
NotificationCenter.default so external-change assertions cannot receive
notifications from other suites.
In `@ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift`:
- Around line 47-71: Introduce one reusable display-UUID availability condition
and apply it via .enabled(if:) to resolvedDisplayUsesExplicitConfiguration and
resolvedDisplayWithoutOverrideFallsBackToGlobalConfiguration in
ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift lines
47-71, removing the try `#require` and early return; apply the same condition to
typedLookupsMatchTheConfiguration and connectedDisplaysAreWellFormed in
ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift lines
130-149 and 242-254, removing their early-return or vacuous-loop gating so
headless environments report skipped tests consistently.
In `@ThawTests/Settings/Models/GeneralSettingsTests.swift`:
- Around line 27-35: Update GeneralSettingsTests to use the injectable Defaults
store: first confirm GeneralSettings reads and writes through Defaults, then
wrap every test with withScratchDefaults. Remove the per-key snapshot and
restore logic and revise the related documentation to describe the scratch-store
isolation instead.
In `@ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift`:
- Around line 38-40: Strengthen the clearing assertion in the hotkey persistence
test by first unwrapping the dictionary returned by Defaults.dictionary(forKey:
.hotkeys), then assert that action.rawValue is absent from the non-optional
dictionary. Avoid optional chaining so a missing dictionary cannot make the
assertion pass.
In `@ThawTests/Settings/Models/ProfileDecodingDefaultsTests.swift`:
- Around line 590-608: Update the documentation above
duplicateIdentifierResolvesToOneSection by either naming the specific report
file or removing the sentence referring to an accompanying report. Keep the
existing explanation about Dictionary iteration order and the test’s assertion
unchanged.
- Around line 732-748: Update applyPreservesLastCustomIceIcon to set
snapshot.iceIcon to a .custom icon before applying it, while retaining a
distinct remembered value in snapshot.lastCustomIceIcon. Use the existing
custom-icon setup from applyGeneralSettingsCustomIconKeepsLastCustom so
iceIcon’s didSet mirror executes and the test distinguishes the assignment
order.
In `@ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift`:
- Around line 31-36: Update the affected doc comments in the
ProfileLayoutLogReplayTests suite to remove the obsolete XCTest “test” prefix
and reference the migrated methods withoutExclusionTheOrphanWouldBeUnmanaged,
buggyCycleDoesNotPlanMoveForUnresolvedOrphan,
loggedDesiredVisibleIsUsedInsteadOfInference, and
displayReconnectNegativeBudgetYieldsNoOverflow.
In `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift`:
- Around line 29-53: Move the shared notifications(named:during:) helper and
NotificationBox class into ThawTests/Support/, then remove both private local
copies from SettingsURIHandlerApplyTests and SettingsURIHandlerCoverageTests.
Preserve their existing behavior and signatures so both test suites reuse the
shared implementation.
In `@ThawTests/Settings/URI/SettingsURIParserTests.swift`:
- Around line 163-177: Update parsingIsTotal to require every test argument to
produce a URL instead of returning silently when URL(string:) is nil. Reuse the
file’s existing helper that enforces URL creation, then pass the resulting URL
to SettingsURIParser.parse.
In `@ThawTests/Shared/DiagnosticLoggerFileTests.swift`:
- Around line 311-324: Update latestLogFileIsSelfConsistent to remove the
cross-read assertion comparing logger.hasLogFiles with latest, since both
accessors independently scan the shared log directory. Retain the existing
latestLogFile shape checks, including the .log extension and logDirectory
parent-path validation.
In `@ThawTests/Support/ProfileTestFixtures.swift`:
- Around line 111-135: The profile test fixtures duplicate encoder and
manifest-seeding logic across three files; centralize it to prevent
configuration drift. In ThawTests/Support/ProfileTestFixtures.swift lines
111-135, add shared makeProfileEncoder() and extend seedManifest with
omittingFilesFor: Set<UUID> = [], skipping those profile files while still
writing the manifest. In
ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift lines 585-624,
remove private seedManifest and makeEncoder, use the shared helpers, and
preserve withManager and writeBundle. In
ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift lines
120-129, replace local JSONEncoder setup in writeProfile with
makeProfileEncoder().
In `@ThawTests/Support/ScratchDefaults.swift`:
- Around line 45-53: Update acquire() to handle task cancellation while waiting
for the lock by wrapping the waiter continuation in withTaskCancellationHandler
and ensuring cancellation resumes or removes the suspended waiter safely.
Preserve the existing immediate-acquisition behavior and prevent cancelled test
tasks from remaining suspended indefinitely.
- Around line 133-143: Unify the synchronization used by the synchronous and
asynchronous ScratchDefaults overloads so they cannot interleave while modifying
the global Defaults.store. Update scratchDefaultsLock and scratchDefaultsMutex
usage so both overloads share one mutual-exclusion mechanism, preserving each
overload’s existing setup, restoration, and cleanup behavior without allowing
one caller to restore another caller’s deleted store.
In `@ThawTests/Utilities/CGImageAnalysisTests.swift`:
- Around line 197-210: Update fallbackAgreesWithFastPath to assert the expected
transparency value for each opaque input in addition to comparing fallback and
normal results. Use opaque to derive the absolute expectation, preserving the
existing agreement assertion.
In `@ThawTests/Utilities/CGImageDetachedCopyTests.swift`:
- Around line 45-63: Update pixelData(of:) to create and use the CGContext
inside data.withUnsafeMutableBytes, passing the buffer pointer as the context
data and performing context.draw within the same closure. Preserve the existing
bitmap configuration, context-creation failure handling, and returned pixel
data.
---
Outside diff comments:
In `@Thaw/Events/HIDEventManager.swift`:
- Around line 1085-1091: Update the sleep guard inside the Task closure in
HIDEventManager to use the Swift-required operator order, placing try? before
await: guard (try? await Task.sleep(...)) != nil. Preserve the existing
cancellation behavior and 250-millisecond delay.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 202-207: Change isDegradedIdentityEnrichmentEnabled from a cached
static let to a computed static property that reads Defaults.store.bool(forKey:
"EnableDegradedItemAXEnrichment") on every access, preserving the existing key
and nonisolated behavior.
- Around line 7051-7057: Update the post-arm abort handling in the functions
containing the unresolved-sourcePID, open-menu, missing-control-item, and
post-Phase-1 control-loss paths to call restoreProfileStateAfterCancelledApply
with the same apply token guard instead of clearProfileState. Preserve
clearProfileState for successful completion, and ensure each cancellation
restores all armed profile state and releases priorProfileApplySnapshot without
persisting changes.
In `@Thaw/Settings/Models/AdvancedSettings.swift`:
- Around line 270-279: Correct the documentation for
sanitizedSearchSectionOrder(from:) to state that empty or entirely unrecognized
input falls back to MenuBarSection.Name.allCases in declaration order, not
Defaults.DefaultValue.searchSectionOrder. Keep the implementation unchanged
unless intentionally aligning its behavior with the documented default.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4245c0bb-afa2-49e9-8370-65b05294d623
📒 Files selected for processing (243)
.github/workflows/ci.yml.gitignore.swiftformatMenuBarItemService/Listener.swiftMenuBarItemService/SourcePIDCache.swiftMenuBarItemService/main.swiftShared/Bridging/Shims.swiftShared/Utilities/DiagnosticLogger.swiftShared/Utilities/MarkerPairResolver.swiftThaw/Events/EventTap.swiftThaw/Events/HIDEventManager.swiftThaw/Events/RunLoopLocalEventMonitor.swiftThaw/Hotkeys/Hotkey.swiftThaw/Hotkeys/HotkeyRegistry.swiftThaw/Main/AppState.swiftThaw/Main/Updates.swiftThaw/MenuBar/Appearance/Configurations/MenuBarAppearanceConfigurationV2.swiftThaw/MenuBar/Appearance/Configurations/MenuBarShapes.swiftThaw/MenuBar/Appearance/MissionControlDetector.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/LayoutBar/LayoutBarPaddingView.swiftThaw/MenuBar/MenuBarItems/AXIdentityCatalog.swiftThaw/MenuBar/MenuBarItems/AXItemActivator.swiftThaw/MenuBar/MenuBarItems/ClickReactionVerifier.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swiftThaw/MenuBar/MenuBarItems/MenuBarItem.swiftThaw/MenuBar/MenuBarItems/MenuBarItemFailureLedger.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swiftThaw/MenuBar/MenuBarItems/MenuBarItemTag.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/MenuBarSection.swiftThaw/MenuBar/MenuBarSectionLayout.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Onboarding/Tour/OnboardingTour.swiftThaw/Onboarding/Tour/TourMockups.swiftThaw/Onboarding/Tour/WelcomeMockup.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/FocusFilterIntent.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/UI/Views/CustomTooltip.swiftThaw/Utilities/ConcurrencyHelpers.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/Extensions.swiftThaw/Utilities/ScreenCapture.swiftThaw/Utilities/SettingsURIHandler.swiftThaw/Utilities/SettingsURIParser.swiftThawTests/AdvancedSettingsTests.swiftThawTests/AutomationSettingsTests.swiftThawTests/CaptureBoundsValidationTests.swiftThawTests/CrossCutting/CoverageSweep1Tests.swiftThawTests/CrossCutting/CoverageSweep2Tests.swiftThawTests/CrossCutting/CoverageSweep3Tests.swiftThawTests/CrossCutting/CoverageSweep4Tests.swiftThawTests/CrossCutting/CoverageSweep5Tests.swiftThawTests/DefaultsKeyTests.swiftThawTests/DiagnosticLoggerTests.swiftThawTests/DisplayIceBarConfigurationTests.swiftThawTests/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/DisplaySettingsManagerMutationTests.swiftThawTests/DisplaySettingsManagerSpacingGateTests.swiftThawTests/ExtensionsTests.swiftThawTests/GeneralSettingsSnapshotTests.swiftThawTests/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyLifecycleTests.swiftThawTests/Hotkeys/KeyCodeTests.swiftThawTests/Hotkeys/KeyCombinationTests.swiftThawTests/Hotkeys/ModifiersTests.swiftThawTests/HotkeysSettingsPersistenceTests.swiftThawTests/IceBarLocationTests.swiftThawTests/IceSettingsImporterTests.swiftThawTests/LayoutResetErrorTests.swiftThawTests/MenuBar/Appearance/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBar/Appearance/MenuBarShapesTests.swiftThawTests/MenuBar/Appearance/MenuBarTintKindTests.swiftThawTests/MenuBar/Appearance/MissionControlDetectorTests.swiftThawTests/MenuBar/ControlItem/ControlItemImageTests.swiftThawTests/MenuBar/ControlItem/ControlItemOcclusionTests.swiftThawTests/MenuBar/ControlItem/ControlItemRecoveryTests.swiftThawTests/MenuBar/IceBar/IceBarLocationTests.swiftThawTests/MenuBar/Items/AXIdentityCatalogTests.swiftThawTests/MenuBar/Items/AXItemActivatorTests.swiftThawTests/MenuBar/Items/AnchorDestinationTests.swiftThawTests/MenuBar/Items/BadgeIndexTests.swiftThawTests/MenuBar/Items/CaptureBoundsValidationTests.swiftThawTests/MenuBar/Items/CapturedImageScaleTests.swiftThawTests/MenuBar/Items/ClickReactionVerifierTests.swiftThawTests/MenuBar/Items/ControlCenterHostedMatchLogReplayTests.swiftThawTests/MenuBar/Items/DisplaySpreadGateTests.swiftThawTests/MenuBar/Items/DivergencePersistenceGateTests.swiftThawTests/MenuBar/Items/FlattenCurrentSectionsTests.swiftThawTests/MenuBar/Items/GhostControlItemWindowTests.swiftThawTests/MenuBar/Items/HiddenDragFailureClassificationTests.swiftThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheDisplayResolutionTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheGatingTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheObservationTests.swiftThawTests/MenuBar/Items/MenuBarItemManagerRearmTests.swiftThawTests/MenuBar/Items/MenuBarItemOrderingTests.swiftThawTests/MenuBar/Items/MenuBarItemServiceTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCodingTests.swiftThawTests/MenuBar/Items/MenuBarItemTagTests.swiftThawTests/MenuBar/Items/MenuBarItemValueTests.swiftThawTests/MenuBar/Items/MenuBarTestFixturesTests.swiftThawTests/MenuBar/Items/MidSectionTransitionTests.swiftThawTests/MenuBar/Items/MoveEventCoordinatesTests.swiftThawTests/MenuBar/Items/MoveFailureBackoffTests.swiftThawTests/MenuBar/Items/MoveMenuGuardTests.swiftThawTests/MenuBar/Items/NewItemsPlacementTests.swiftThawTests/MenuBar/Items/PartitionUnmanagedUIDsTests.swiftThawTests/MenuBar/Items/PendingRehideTagIdentifiersTests.swiftThawTests/MenuBar/Items/SavedLayoutSectionLookupTests.swiftThawTests/MenuBar/Items/SavedPositionLookupTests.swiftThawTests/MenuBar/Items/SectionGeometryGateTests.swiftThawTests/MenuBar/Items/SelectWindowForBatchScanTests.swiftThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swiftThawTests/MenuBar/Items/SourcePIDResolutionGateTests.swiftThawTests/MenuBar/Items/WindowIDsChangedGateTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerUnmanagedPlacementTests.swiftThawTests/MenuBar/Layout/LayoutResetErrorTests.swiftThawTests/MenuBar/Layout/LayoutResetReentrancyTests.swiftThawTests/MenuBar/Layout/NotchOverflowRevealTests.swiftThawTests/MenuBar/Layout/PlanFullSortSequenceTests.swiftThawTests/MenuBar/Layout/PlanLCSMoveSequenceTests.swiftThawTests/MenuBar/Layout/PlanLeftmostMoveTests.swiftThawTests/MenuBar/Layout/PlanNotchOverflowTests.swiftThawTests/MenuBar/Layout/PlanPendingMoveTests.swiftThawTests/MenuBar/Layout/PlanSectionOrderTests.swiftThawTests/MenuBar/Layout/PlanUnmanagedPlacementTests.swiftThawTests/MenuBar/Section/MenuBarSectionGeometryTests.swiftThawTests/MenuBar/Section/MenuBarSectionNameTests.swiftThawTests/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBarItemServiceTests.swiftThawTests/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBarItemTagTests.swiftThawTests/MenuBarSectionNameTests.swiftThawTests/MenuBarShapesTests.swiftThawTests/MenuBarTestFixturesTests.swiftThawTests/ModifiersTests.swiftThawTests/MoveFailureBackoffTests.swiftThawTests/Onboarding/OnboardingMockupsTests.swiftThawTests/Onboarding/OnboardingSlideTests.swiftThawTests/OnboardingMockupsTests.swiftThawTests/OnboardingSlideTests.swiftThawTests/PermissionTests.swiftThawTests/Permissions/PermissionTests.swiftThawTests/ProfileEntityTests.swiftThawTests/ProfileExportBundleTests.swiftThawTests/ProfileManagerDeleteTests.swiftThawTests/ProfileManagerRearmGateTests.swiftThawTests/ProfileTests.swiftThawTests/RehideStrategyTests.swiftThawTests/SectionDividerStyleTests.swiftThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/AutomationSettingsTests.swiftThawTests/Settings/Models/DisplayIceBarConfigurationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/Settings/Models/DisplaySettingsManagerLookupTests.swiftThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerSpacingGateTests.swiftThawTests/Settings/Models/DisplaySettingsManagerURINotificationTests.swiftThawTests/Settings/Models/ExternalSettingsChangeTests.swiftThawTests/Settings/Models/FocusFilterIntentTests.swiftThawTests/Settings/Models/GeneralSettingsSnapshotTests.swiftThawTests/Settings/Models/GeneralSettingsTests.swiftThawTests/Settings/Models/HookScriptTests.swiftThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swiftThawTests/Settings/Models/ProfileApplyInFlightFlagTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/Models/ProfileEntityTests.swiftThawTests/Settings/Models/ProfileExportBundleTests.swiftThawTests/Settings/Models/ProfileLayoutLogReplayTests.swiftThawTests/Settings/Models/ProfileManagerCRUDTests.swiftThawTests/Settings/Models/ProfileManagerDeepTests.swiftThawTests/Settings/Models/ProfileManagerDeleteTests.swiftThawTests/Settings/Models/ProfileManagerObservationTasksTests.swiftThawTests/Settings/Models/ProfileManagerPersistenceTests.swiftThawTests/Settings/Models/ProfileManagerRearmGateTests.swiftThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swiftThawTests/Settings/Models/ProfileTests.swiftThawTests/Settings/Models/RehideStrategyTests.swiftThawTests/Settings/Models/SectionDividerStyleTests.swiftThawTests/Settings/Models/SettingsResetterTests.swiftThawTests/Settings/Search/SearchIndexTests.swiftThawTests/Settings/Search/SearchModelTests.swiftThawTests/Settings/Search/SettingsSearchNavigationTests.swiftThawTests/Settings/URI/IntegrationURIContractTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Settings/URI/SettingsURIHandlerGetTests.swiftThawTests/Settings/URI/SettingsURIHandlerTailTests.swiftThawTests/Settings/URI/SettingsURIHandlerTests.swiftThawTests/Settings/URI/SettingsURIParserTests.swiftThawTests/SettingsResetterTests.swiftThawTests/SettingsURIHandlerApplyTests.swiftThawTests/SettingsURIHandlerGetTests.swiftThawTests/SettingsURIHandlerTests.swiftThawTests/Shared/CodeSigningInfoTests.swiftThawTests/Shared/DiagnosticLoggerFileTests.swiftThawTests/Shared/DiagnosticLoggerTests.swiftThawTests/Shared/MarkerPairResolverTests.swiftThawTests/Shared/SharedExtensionsTests.swiftThawTests/Shared/WindowInfoDecodingTests.swiftThawTests/Shared/WindowInfoDerivedTests.swiftThawTests/Shared/WindowInfoTests.swiftThawTests/SharedExtensionsTests.swiftThawTests/Support/GraphicsTestFixtures.swiftThawTests/Support/MenuBarTestFixtures.swiftThawTests/Support/ProfileTestFixtures.swiftThawTests/Support/ScratchDefaults.swiftThawTests/Utilities/CGImageAnalysisTests.swiftThawTests/Utilities/CGImageDetachedCopyTests.swiftThawTests/Utilities/ConcurrencyHelpersTests.swiftThawTests/Utilities/CustomTooltipPanelTests.swiftThawTests/Utilities/DefaultsKeyTests.swiftThawTests/Utilities/ExtensionsCoverageTests.swiftThawTests/Utilities/ExtensionsGraphicsTests.swiftThawTests/Utilities/ExtensionsRemainderTests.swiftThawTests/Utilities/ExtensionsTests.swiftThawTests/Utilities/HookRunnerTests.swiftThawTests/Utilities/IceSettingsImporterTailTests.swiftThawTests/Utilities/IceSettingsImporterTests.swiftThawTests/Utilities/LocalizedErrorWrapperTests.swiftThawTests/Utilities/MigrationTests.swiftThawTests/Utilities/MouseMovedThrottleTests.swiftThawTests/Utilities/ObjectStorageTests.swiftThawTests/Utilities/PredicatesTests.swiftThawTests/Utilities/ShareableContentCacheTests.swiftThawTests/Utilities/SimpleSemaphoreTests.swiftThawTests/WindowInfoTests.swiftdocs/ASSURANCE_CASE.mdscripts/coverage-report.pyscripts/swiftlint-inputs.xcfilelistsonar-project.properties
💤 Files with no reviewable changes (28)
- ThawTests/DiagnosticLoggerTests.swift
- ThawTests/GeneralSettingsSnapshotTests.swift
- ThawTests/ModifiersTests.swift
- ThawTests/IceBarLocationTests.swift
- ThawTests/LayoutResetErrorTests.swift
- ThawTests/ExtensionsTests.swift
- ThawTests/MenuBarTestFixturesTests.swift
- ThawTests/AdvancedSettingsTests.swift
- ThawTests/DisplayIceBarConfigurationTests.swift
- ThawTests/HotkeysSettingsPersistenceTests.swift
- ThawTests/IceSettingsImporterTests.swift
- ThawTests/MoveFailureBackoffTests.swift
- ThawTests/DisplaySettingsManagerGlobalFallbackTests.swift
- ThawTests/MenuBarSectionNameTests.swift
- ThawTests/DisplaySettingsManagerMutationTests.swift
- ThawTests/AutomationSettingsTests.swift
- ThawTests/MenuBarItemTagCanonicalizationTests.swift
- Thaw/MenuBar/Appearance/MissionControlDetector.swift
- ThawTests/DefaultsKeyTests.swift
- ThawTests/DisplaySettingsManagerSpacingGateTests.swift
- ThawTests/CaptureBoundsValidationTests.swift
- Thaw/MenuBar/MenuBarSection.swift
- ThawTests/MenuBarShapesTests.swift
- ThawTests/MenuBarItemTagTests.swift
- ThawTests/MenuBar/Items/GhostControlItemWindowTests.swift
- ThawTests/MenuBarItemServiceTests.swift
- ThawTests/MenuBarAppearanceConfigurationTests.swift
- ThawTests/HotkeyActionTests.swift
Comment on lines
+125
to
+151
| # Files under Shared/ are compiled into both the app and the XPC service, so | ||
| # xccov reports them once per target. Sonar counts each source file once, so | ||
| # keep the best result per path -- a line covered by either target is covered. | ||
| best: dict[str, dict] = {} | ||
| excluded_files = 0 | ||
| for target in report.get("targets", []): | ||
| for entry in target.get("files", []): | ||
| path = entry["path"] | ||
| # xccov reports absolute paths; make them repo-relative. | ||
| marker = str(REPO_ROOT) + "/" | ||
| path = path[len(marker):] if path.startswith(marker) else path | ||
| if not path.startswith(sources): | ||
| continue | ||
| if is_excluded(path, exclusions): | ||
| excluded_files += 1 | ||
| continue | ||
| lines = entry["executableLines"] | ||
| covered = entry["coveredLines"] | ||
| previous = best.get(path) | ||
| if previous is None or covered > previous["covered"]: | ||
| best[path] = { | ||
| "path": path, | ||
| "lines": lines, | ||
| "covered": covered, | ||
| "uncovered": lines - covered, | ||
| "coverage": round(100 * covered / lines, 1) if lines else 100.0, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
State the limit of the per-file dedup rule.
The comment says a line covered by either target is covered, but the code keeps the whole entry with the largest coveredLines rather than a union of covered lines. If the two targets cover different line sets, or report different executableLines, the reported figure comes from one entry only. The result stays close to Sonar's, so this is an accuracy note, not a defect. Adjust the comment to describe the approximation.
🤖 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 `@scripts/coverage-report.py` around lines 125 - 151, Update the explanatory
comment above the `best` map and per-file deduplication loop to state that the
implementation approximates Sonar by retaining the entry with the greatest
`coveredLines`, rather than unioning covered or executable line sets across
targets. Preserve the existing deduplication logic.
| Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift,\ | ||
| Thaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swift,\ | ||
| Thaw/MenuBar/MenuBarItems/AXItemActivator.swift,\ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Excluding AXItemActivator.swift discards the coverage from its new test suite.
This PR adds ThawTests/MenuBar/Items/AXItemActivatorTests.swift. Line 85 now excludes Thaw/MenuBar/MenuBarItems/AXItemActivator.swift from coverage measurement, so whatever that suite exercises no longer counts.
That also conflicts with the rule this file states at Lines 22-26: pure logic stays measured, and files that bury both kinds of code are split rather than excluded whole. MenuBarSection and MenuBarItem were handled that way in this same PR; AXItemActivator was not.
Either split the file along the same seam and exclude only the AX-interaction half, or drop the exclusion. If the whole file is genuinely AX-driven, state why the new test suite exists and record the reason in the comment block above, as the other whole-file exclusions do.
#!/bin/bash
# Compare what the new suite exercises against the excluded file's contents.
fd -g 'AXItemActivator*.swift' --exec ast-grep outline {} --items all
rg -n 'AXItemActivator\.' ThawTests🤖 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 `@sonar-project.properties` at line 85, Remove the whole-file coverage
exclusion for AXItemActivator.swift unless it is genuinely entirely AX-driven;
preserve coverage for the pure logic exercised by AXItemActivatorTests. If the
file cannot be split, document the specific AX-only rationale in the exclusion
comment block above, consistent with the existing whole-file exclusions.
Comment on lines
+58
to
+62
| return Defaults.store.bool(forKey: "AllowsBetaUpdates") | ||
| } | ||
| set { | ||
| withMutation(keyPath: \.allowsBetaUpdates) { | ||
| UserDefaults.standard.set(newValue, forKey: "AllowsBetaUpdates") | ||
| Defaults.store.set(newValue, forKey: "AllowsBetaUpdates") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the centralized Defaults.Key for beta updates.
Lines 58-62 and Line 180 duplicate "AllowsBetaUpdates" and bypass the typed Defaults interface. Use Defaults.bool(forKey: .allowsBetaUpdates) and Defaults.set(_:forKey: .allowsBetaUpdates) so reads and writes share the centralized key.
Proposed fix
- return Defaults.store.bool(forKey: "AllowsBetaUpdates")
+ return Defaults.bool(forKey: .allowsBetaUpdates)
- Defaults.store.set(newValue, forKey: "AllowsBetaUpdates")
+ Defaults.set(newValue, forKey: .allowsBetaUpdates)
- if Defaults.store.bool(forKey: "AllowsBetaUpdates") {
+ if Defaults.bool(forKey: .allowsBetaUpdates) {Also applies to: 180-180
🤖 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 `@Thaw/Main/Updates.swift` around lines 58 - 62, Update the allowsBetaUpdates
property getter and setter to use the centralized typed Defaults interface:
replace the raw "AllowsBetaUpdates" accesses with Defaults.bool(forKey:
.allowsBetaUpdates) and Defaults.set(_:forKey: .allowsBetaUpdates), including
the additional occurrence near the related update settings code.
Comment on lines
+132
to
+149
| for (_, indices) in groups where indices.count > 1 { | ||
| let sorted = indices.sorted { items[$0].windowID < items[$1].windowID } | ||
| for (instanceIndex, itemIndex) in sorted.enumerated() where instanceIndex > 0 { | ||
| if let sourcePID = items[itemIndex].sourcePID { | ||
| items[itemIndex] = MenuBarItem( | ||
| uncheckedItemWindow: windows[itemIndex], | ||
| sourcePID: sourcePID, | ||
| instanceIndex: instanceIndex | ||
| ) | ||
| } else { | ||
| items[itemIndex] = MenuBarItem( | ||
| uncheckedItemWindow: windows[itemIndex], | ||
| sourcePID: nil, | ||
| instanceIndex: instanceIndex | ||
| ) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Collapse the duplicated branches in the instance-index rebuild.
items[itemIndex].sourcePID is already pid_t?. Both branches call the same initializer with the same arguments; only the sourcePID value differs. Pass the optional directly.
♻️ Proposed refactor
for (instanceIndex, itemIndex) in sorted.enumerated() where instanceIndex > 0 {
- if let sourcePID = items[itemIndex].sourcePID {
- items[itemIndex] = MenuBarItem(
- uncheckedItemWindow: windows[itemIndex],
- sourcePID: sourcePID,
- instanceIndex: instanceIndex
- )
- } else {
- items[itemIndex] = MenuBarItem(
- uncheckedItemWindow: windows[itemIndex],
- sourcePID: nil,
- instanceIndex: instanceIndex
- )
- }
+ items[itemIndex] = MenuBarItem(
+ uncheckedItemWindow: windows[itemIndex],
+ sourcePID: items[itemIndex].sourcePID,
+ instanceIndex: instanceIndex
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (_, indices) in groups where indices.count > 1 { | |
| let sorted = indices.sorted { items[$0].windowID < items[$1].windowID } | |
| for (instanceIndex, itemIndex) in sorted.enumerated() where instanceIndex > 0 { | |
| if let sourcePID = items[itemIndex].sourcePID { | |
| items[itemIndex] = MenuBarItem( | |
| uncheckedItemWindow: windows[itemIndex], | |
| sourcePID: sourcePID, | |
| instanceIndex: instanceIndex | |
| ) | |
| } else { | |
| items[itemIndex] = MenuBarItem( | |
| uncheckedItemWindow: windows[itemIndex], | |
| sourcePID: nil, | |
| instanceIndex: instanceIndex | |
| ) | |
| } | |
| } | |
| } | |
| for (_, indices) in groups where indices.count > 1 { | |
| let sorted = indices.sorted { items[$0].windowID < items[$1].windowID } | |
| for (instanceIndex, itemIndex) in sorted.enumerated() where instanceIndex > 0 { | |
| items[itemIndex] = MenuBarItem( | |
| uncheckedItemWindow: windows[itemIndex], | |
| sourcePID: items[itemIndex].sourcePID, | |
| instanceIndex: instanceIndex | |
| ) | |
| } | |
| } |
🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift around lines 132 -
149, In the instance-index rebuild loop within the enumeration logic, replace
the duplicated sourcePID branches with one MenuBarItem initializer that passes
items[itemIndex].sourcePID directly, while preserving the existing
uncheckedItemWindow and instanceIndex arguments.
Comment on lines
+5207
to
5215
| // The anchor math below treats index adjacency as physical | ||
| // adjacency, but the item list arrives in Window Server order. | ||
| // Sort by each item's leading edge so successor/predecessor mean | ||
| // the item's actual on-screen neighbors. | ||
| let orderedItems = items.sorted { $0.bounds.minX < $1.bounds.minX } | ||
|
|
||
| guard let index = orderedItems.firstIndex(matching: item.tag) else { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a deterministic tie-break to the leading-edge sort.
sorted(by:) is not stable in Swift, and this call site resolves neighbors for items in the hidden or always-hidden section. Those items are parked off-screen horizontally, so two of them can report the same or near-identical bounds.minX. When they tie, the relative order of orderedItems is unspecified, so anchors.successor can select a different anchor on two runs over the same bar. The rehide then returns the item to a different slot each time.
A secondary key on windowID makes the order total and reproducible.
🐛 Proposed fix
- let orderedItems = items.sorted { $0.bounds.minX < $1.bounds.minX }
+ let orderedItems = items.sorted {
+ if $0.bounds.minX != $1.bounds.minX {
+ return $0.bounds.minX < $1.bounds.minX
+ }
+ return $0.windowID < $1.windowID
+ }Confirm whether off-screen items in the same section can share a leading edge.
#!/bin/bash
# Look for the off-screen parking coordinate used for hidden items.
rg -n -C3 'pushed far off-screen|offscreen|offScreen|x=-1|origin\.x == -1' --type=swift Thaw/MenuBar🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 5207 - 5215,
Update the orderedItems sort in the neighbor-resolution flow to use windowID as
a deterministic secondary key whenever bounds.minX values are equal, producing a
total reproducible order while preserving leading-edge ordering as the primary
key.
Comment on lines
+29
to
+53
| /// Collects the notifications posted on `name` while `body` runs. | ||
| private func notifications( | ||
| named name: Notification.Name, | ||
| during body: () -> Void | ||
| ) -> [Notification] { | ||
| let box = NotificationBox() | ||
| let token = NotificationCenter.default.addObserver( | ||
| forName: name, | ||
| object: nil, | ||
| queue: nil | ||
| ) { box.append($0) } | ||
| defer { NotificationCenter.default.removeObserver(token) } | ||
| body() | ||
| return box.contents | ||
| } | ||
|
|
||
| /// Notifications are delivered synchronously on the posting thread, but | ||
| /// the observer closure is `@Sendable`, so the collector needs to be a | ||
| /// reference type rather than a captured `var`. | ||
| private final class NotificationBox: @unchecked Sendable { | ||
| private(set) var contents: [Notification] = [] | ||
| func append(_ notification: Notification) { | ||
| contents.append(notification) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move the notification collector into the shared support target.
notifications(named:during:) and NotificationBox exist here and again in ThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swift at lines 17-41, with identical bodies. Both copies are private, so neither suite can reuse the other. This PR already established ThawTests/Support/ for shared fixtures. Put one copy there and delete both local copies.
🤖 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 `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift` around lines 29 -
53, Move the shared notifications(named:during:) helper and NotificationBox
class into ThawTests/Support/, then remove both private local copies from
SettingsURIHandlerApplyTests and SettingsURIHandlerCoverageTests. Preserve their
existing behavior and signatures so both test suites reuse the shared
implementation.
Comment on lines
+111
to
+135
| func seedManifest(with profiles: [Profile], into directory: URL) throws { | ||
| let fileManager = FileManager.default | ||
| try fileManager.createDirectory(at: directory, withIntermediateDirectories: true) | ||
|
|
||
| let encoder = JSONEncoder() | ||
| encoder.dateEncodingStrategy = .iso8601 | ||
| encoder.outputFormatting = [.prettyPrinted, .sortedKeys] | ||
|
|
||
| for profile in profiles { | ||
| let data = try encoder.encode(profile) | ||
| try data.write( | ||
| to: directory.appendingPathComponent("\(profile.id.uuidString).json"), | ||
| options: .atomic | ||
| ) | ||
| } | ||
|
|
||
| let metadata = profiles.map { | ||
| ProfileMetadata(id: $0.id, name: $0.name, createdAt: $0.createdAt, modifiedAt: $0.modifiedAt) | ||
| } | ||
| let manifestData = try encoder.encode(metadata) | ||
| try manifestData.write( | ||
| to: directory.appendingPathComponent("profiles.json"), | ||
| options: .atomic | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The profile encoder and manifest seeding are centralized only partially. Three files each configure the same JSONEncoder (.iso8601, [.prettyPrinted, .sortedKeys]) and two of them seed profile JSON. That encoder configuration must mirror the one ProfileManager builds in its initializer, or seeded files stop decoding through the manager's decoder. Three independent copies can drift apart silently.
ThawTests/Support/ProfileTestFixtures.swift#L111-L135: extract the encoder configuration into a sharedmakeProfileEncoder()factory, and add anomittingFilesFor: Set<UUID> = []parameter toseedManifestso it also covers the missing-file case.ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift#L585-L624: delete the privateseedManifestandmakeEncoder, and call the sharedseedManifest(with:omittingFilesFor:into:)andmakeProfileEncoder()instead. KeepwithManagerandwriteBundle.ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift#L120-L129: replace the local encoder configuration inwriteProfilewithmakeProfileEncoder().
📍 Affects 3 files
ThawTests/Support/ProfileTestFixtures.swift#L111-L135(this comment)ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift#L585-L624ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift#L120-L129
🤖 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 `@ThawTests/Support/ProfileTestFixtures.swift` around lines 111 - 135, The
profile test fixtures duplicate encoder and manifest-seeding logic across three
files; centralize it to prevent configuration drift. In
ThawTests/Support/ProfileTestFixtures.swift lines 111-135, add shared
makeProfileEncoder() and extend seedManifest with omittingFilesFor: Set<UUID> =
[], skipping those profile files while still writing the manifest. In
ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift lines 585-624,
remove private seedManifest and makeEncoder, use the shared helpers, and
preserve withManager and writeBundle. In
ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift lines
120-129, replace local JSONEncoder setup in writeProfile with
makeProfileEncoder().
Comment on lines
+45
to
+53
| func acquire() async { | ||
| if !isLocked { | ||
| isLocked = true | ||
| return | ||
| } | ||
| await withCheckedContinuation { continuation in | ||
| waiters.append(continuation) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
acquire() ignores cancellation, so a cancelled test hangs instead of failing.
withCheckedContinuation has no cancellation handler. If the enclosing test task is cancelled while a waiter is suspended, the continuation is never resumed and the test never returns. Use withTaskCancellationHandler, or document that callers must not cancel.
🤖 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 `@ThawTests/Support/ScratchDefaults.swift` around lines 45 - 53, Update
acquire() to handle task cancellation while waiting for the lock by wrapping the
waiter continuation in withTaskCancellationHandler and ensuring cancellation
resumes or removes the suspended waiter safely. Preserve the existing
immediate-acquisition behavior and prevent cancelled test tasks from remaining
suspended indefinitely.
Comment on lines
+133
to
+143
| await scratchDefaultsMutex.acquire() | ||
| let previous = Defaults.store | ||
| Defaults.store = suite | ||
| defer { | ||
| // The restore and cleanup below run synchronously before the release | ||
| // task is spawned, so the next acquirer can only ever see the | ||
| // restored store. | ||
| Defaults.store = previous | ||
| suite.removePersistentDomain(forName: suiteName) | ||
| Task { await scratchDefaultsMutex.release() } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The two overloads still do not exclude each other, so a cross-suite interleave can publish a deleted store.
The async overload serializes through ScratchDefaultsMutex. The synchronous overload serializes through scratchDefaultsLock. The doc comments at lines 37-40 and 119-121 state that the two are independent. Defaults.store is one global, so a synchronous caller in suite A and an async caller in suite B can interleave their save-install-restore windows. The async caller then restores a store that suite A has already deleted with removePersistentDomain, and every later test reads through a dead store.
.serialized does not close this gap, because it orders tests inside one suite only. ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift uses both overloads, so both lock domains are live in this test target.
Route both overloads through one mutual-exclusion mechanism. Options: make every caller use the async overload, or make the store swap task-local rather than global so no caller has to restore a value another caller owns.
🤖 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 `@ThawTests/Support/ScratchDefaults.swift` around lines 133 - 143, Unify the
synchronization used by the synchronous and asynchronous ScratchDefaults
overloads so they cannot interleave while modifying the global Defaults.store.
Update scratchDefaultsLock and scratchDefaultsMutex usage so both overloads
share one mutual-exclusion mechanism, preserving each overload’s existing setup,
restoration, and cleanup behavior without allowing one caller to restore another
caller’s deleted store.
Comment on lines
+45
to
63
| private func pixelData(of image: CGImage) throws -> [UInt8] { | ||
| let width = image.width | ||
| let height = image.height | ||
| var data = [UInt8](repeating: 0, count: width * height * 4) | ||
| let colorSpace = CGColorSpaceCreateDeviceRGB() | ||
| let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue | ||
| let context = CGContext( | ||
| let bitmapContext = CGContext( | ||
| data: &data, | ||
| width: width, | ||
| height: height, | ||
| bitsPerComponent: 8, | ||
| bytesPerRow: width * 4, | ||
| space: colorSpace, | ||
| bitmapInfo: bitmapInfo | ||
| )! | ||
| ) | ||
| let context = try #require(bitmapContext, "Could not create a bitmap context") | ||
| context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height)) | ||
| return data | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
pixelData(of:) hands CGContext a pointer into a Swift array that outlives the call.
&data produces a pointer that is only guaranteed valid for the duration of the CGContext initializer call. context.draw writes through that pointer afterwards. The array storage may be moved or copied in between, so the reads at Line 63 are not guaranteed to observe the drawn pixels. Wrap the context creation and the draw in withUnsafeMutableBytes so the buffer stays fixed for the whole lifetime of the context.
♻️ Proposed change
private func pixelData(of image: CGImage) throws -> [UInt8] {
let width = image.width
let height = image.height
var data = [UInt8](repeating: 0, count: width * height * 4)
let colorSpace = CGColorSpaceCreateDeviceRGB()
let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue
- let bitmapContext = CGContext(
- data: &data,
- width: width,
- height: height,
- bitsPerComponent: 8,
- bytesPerRow: width * 4,
- space: colorSpace,
- bitmapInfo: bitmapInfo
- )
- let context = try `#require`(bitmapContext, "Could not create a bitmap context")
- context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height))
+ try data.withUnsafeMutableBytes { buffer in
+ let bitmapContext = CGContext(
+ data: buffer.baseAddress,
+ width: width,
+ height: height,
+ bitsPerComponent: 8,
+ bytesPerRow: width * 4,
+ space: colorSpace,
+ bitmapInfo: bitmapInfo
+ )
+ let context = try `#require`(bitmapContext, "Could not create a bitmap context")
+ context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height))
+ }
return data
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func pixelData(of image: CGImage) throws -> [UInt8] { | |
| let width = image.width | |
| let height = image.height | |
| var data = [UInt8](repeating: 0, count: width * height * 4) | |
| let colorSpace = CGColorSpaceCreateDeviceRGB() | |
| let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue | |
| let context = CGContext( | |
| let bitmapContext = CGContext( | |
| data: &data, | |
| width: width, | |
| height: height, | |
| bitsPerComponent: 8, | |
| bytesPerRow: width * 4, | |
| space: colorSpace, | |
| bitmapInfo: bitmapInfo | |
| )! | |
| ) | |
| let context = try #require(bitmapContext, "Could not create a bitmap context") | |
| context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height)) | |
| return data | |
| } | |
| private func pixelData(of image: CGImage) throws -> [UInt8] { | |
| let width = image.width | |
| let height = image.height | |
| var data = [UInt8](repeating: 0, count: width * height * 4) | |
| let colorSpace = CGColorSpaceCreateDeviceRGB() | |
| let bitmapInfo = CGImageAlphaInfo.premultipliedLast.rawValue | |
| try data.withUnsafeMutableBytes { buffer in | |
| let bitmapContext = CGContext( | |
| data: buffer.baseAddress, | |
| width: width, | |
| height: height, | |
| bitsPerComponent: 8, | |
| bytesPerRow: width * 4, | |
| space: colorSpace, | |
| bitmapInfo: bitmapInfo | |
| ) | |
| let context = try `#require`(bitmapContext, "Could not create a bitmap context") | |
| context.draw(image, in: CGRect(x: 0, y: 0, width: width, height: height)) | |
| } | |
| return data | |
| } |
🤖 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 `@ThawTests/Utilities/CGImageDetachedCopyTests.swift` around lines 45 - 63,
Update pixelData(of:) to create and use the CGContext inside
data.withUnsafeMutableBytes, passing the buffer pointer as the context data and
performing context.draw within the same closure. Preserve the existing bitmap
configuration, context-creation failure handling, and returned pixel data.
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (2)
202-207: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the enrichment gate read
Defaults.storeon each access.
static nonisolated letinitializes once, on first access, and caches the value for the process lifetime.Defaults.storeis now injectable, so the cached value depends on which store happens to be installed when some test first touches this property. A test that installs a scratch store afterwards reads the stale flag, and a test that touches the property while a scratch store is installed bakes that scratch value in for every later test in the same process.The flag only gates a diagnostic pass, so re-reading it per access costs one
UserDefaultslookup per cache cycle.🛡️ Proposed fix
- static nonisolated let isDegradedIdentityEnrichmentEnabled = - Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") + static nonisolated var isDegradedIdentityEnrichmentEnabled: Bool { + Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") + }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 202 - 207, Change isDegradedIdentityEnrichmentEnabled from a cached static let to a computed static property that reads Defaults.store.bool(forKey: "EnableDegradedItemAXEnrichment") on every access, preserving the existing key and nonisolated behavior.
7051-7057: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAbort paths leave the never-committed profile spec armed in memory.
armProfileStateoverwritespinnedHiddenBundleIDs,pinnedAlwaysHiddenBundleIDs,savedSectionOrder,activeProfileLayout, andactiveProfileItemIdentifiers, and defers disk persistence topersistProfileStateOnSuccess. The newrestoreProfileStateAfterCancelledApplyundoes that overwrite, but only Phase 7 cancellation calls it (Line 8088).Four post-arm aborts call
clearProfileStateinstead, which clearsisApplyingProfileLayoutand nothing else:
- Line 7222: unresolved-sourcePID majority.
- Line 7231: a menu bar item menu is open.
- Line 7243: missing control items.
- Line 7955: control items lost after Phase 1.
No moves ran and nothing was persisted on any of these paths, so memory now describes a profile that disk does not hold. That is the exact condition the snapshot doc comment at Lines 495-500 names: the late-arrival re-sort path keeps sorting toward a profile that never committed.
priorProfileApplySnapshotalso stays retained until the next arm replaces it.Roll back on these aborts as well, using the same token guard.
🐛 Proposed fix
/// Profile-only exit cleanup: refresh the sorted snapshot and clear /// the in-flight profile flag. No-op for .savedOrder. private func clearProfileState(source: ApplySource, items: [MenuBarItem]) { updateProfileSortedSnapshot(source: source, items: items) guard case .profile = source else { return } isApplyingProfileLayout = false } + + /// Exit cleanup for a profile apply that aborted after arming without + /// moving anything and without persisting. Reverts the armed spec so + /// memory matches what disk still holds, then runs the normal teardown. + private func abandonProfileApply( + source: ApplySource, + items: [MenuBarItem], + token: Int + ) { + if case .profile = source { + restoreProfileStateAfterCancelledApply(token: token) + } + clearProfileState(source: source, items: items) + }Then replace the four abort calls, for example at Line 7231:
if await isAnyMenuBarItemMenuOpen() { MenuBarItemManager.diagLog.info("applyProfileLayout: skipping, a menu bar item menu is open") - clearProfileState(source: source, items: items) + abandonProfileApply(source: source, items: items, token: applyToken) return }🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 7051 - 7057, Update the post-arm abort handling in the functions containing the unresolved-sourcePID, open-menu, missing-control-item, and post-Phase-1 control-loss paths to call restoreProfileStateAfterCancelledApply with the same apply token guard instead of clearProfileState. Preserve clearProfileState for successful completion, and ensure each cancellation restores all armed profile state and releases priorProfileApplySnapshot without persisting changes.Thaw/Events/HIDEventManager.swift (1)
1085-1091: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the documented
try? awaitorder for the sleep guard.Swift requires
trybeforeawaitin expressions that use both operators.guard await (try? Task.sleep(...))violates that order; useguard (try? await Task.sleep(...))so the line is guaranteed to compile and matches the existingtry await Task.sleep(...)usage pattern.🤖 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 `@Thaw/Events/HIDEventManager.swift` around lines 1085 - 1091, Update the sleep guard inside the Task closure in HIDEventManager to use the Swift-required operator order, placing try? before await: guard (try? await Task.sleep(...)) != nil. Preserve the existing cancellation behavior and 250-millisecond delay.Thaw/Settings/Models/AdvancedSettings.swift (1)
270-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCorrect the doc comment: this function never returns the default order.
The doc states "Returns the default order if the input is unusable." The implementation does not do that. When
rawValuesis empty or holds only unrecognized values,preferredis empty and the result isMenuBarSection.Name.allCases, which is the declaration order rather thanDefaults.DefaultValue.searchSectionOrder.
SettingsResetterTests.resetAdvancedRestoresSearchSectionOrderderives its expectation by calling this same function, so it cannot detect the difference. Either update the doc to describe theallCasesfallback, or make the empty-input case return the sanitized default order.📝 Proposed doc correction
/// Returns a search-section order that contains each `MenuBarSection.Name` /// case exactly once, using the supplied raw values as the preferred order - /// and filling any missing cases at the end. Returns the default order if - /// the input is unusable. + /// and filling any missing cases at the end in `allCases` order. Unusable + /// raw values are dropped, so an input with no recognized value yields + /// `MenuBarSection.Name.allCases`. static func sanitizedSearchSectionOrder(from rawValues: [String]) -> [MenuBarSection.Name] {🤖 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 `@Thaw/Settings/Models/AdvancedSettings.swift` around lines 270 - 279, Correct the documentation for sanitizedSearchSectionOrder(from:) to state that empty or entirely unrecognized input falls back to MenuBarSection.Name.allCases in declaration order, not Defaults.DefaultValue.searchSectionOrder. Keep the implementation unchanged unless intentionally aligning its behavior with the documented default.
🤖 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 `@scripts/coverage-report.py`:
- Around line 125-151: Update the explanatory comment above the `best` map and
per-file deduplication loop to state that the implementation approximates Sonar
by retaining the entry with the greatest `coveredLines`, rather than unioning
covered or executable line sets across targets. Preserve the existing
deduplication logic.
In `@sonar-project.properties`:
- Line 85: Remove the whole-file coverage exclusion for AXItemActivator.swift
unless it is genuinely entirely AX-driven; preserve coverage for the pure logic
exercised by AXItemActivatorTests. If the file cannot be split, document the
specific AX-only rationale in the exclusion comment block above, consistent with
the existing whole-file exclusions.
In `@Thaw/Main/Updates.swift`:
- Around line 58-62: Update the allowsBetaUpdates property getter and setter to
use the centralized typed Defaults interface: replace the raw
"AllowsBetaUpdates" accesses with Defaults.bool(forKey: .allowsBetaUpdates) and
Defaults.set(_:forKey: .allowsBetaUpdates), including the additional occurrence
near the related update settings code.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItem`+Enumeration.swift:
- Around line 132-149: In the instance-index rebuild loop within the enumeration
logic, replace the duplicated sourcePID branches with one MenuBarItem
initializer that passes items[itemIndex].sourcePID directly, while preserving
the existing uncheckedItemWindow and instanceIndex arguments.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 5207-5215: Update the orderedItems sort in the neighbor-resolution
flow to use windowID as a deterministic secondary key whenever bounds.minX
values are equal, producing a total reproducible order while preserving
leading-edge ordering as the primary key.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemTag.swift`:
- Line 9: Confirm that the package configuration declares the swift-algorithms
dependency required by the Algorithms import in MenuBarItemTag. Ensure the
target containing MenuBarItemTag links that product, distinguishing it from
swift-async-algorithms, and preserve the order-preserving deduplication
implementation.
In `@Thaw/MenuBar/Search/MenuBarSearchPanel.swift`:
- Around line 401-402: Remove the deprecated Defaults.store.synchronize() call
from the panel frame save path, while preserving the existing
Defaults.store.set(...) frame persistence.
In `@Thaw/Settings/Models/Profile.swift`:
- Around line 334-347: Update resolvedItemSectionMap to treat a non-nil empty
itemSectionMap as absent and fall back to the saved section map, matching
resolvedItemOrder. Also update captureCurrentLayout so empty derived itemOrder
and itemSectionMap collections are not persisted over existing valid saved
values.
- Around line 71-76: Update the snapshot-application logic around the
lastCustomIceIcon assignment in Profile so a nil snapshot value does not
overwrite the value populated by the .custom iceIcon didSet. Apply
lastCustomIceIcon only when the snapshot contains a value, while preserving the
reordered assignment for snapshots that provide one.
In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 652-660: Update ProfileManager.captureCurrentLayout() to read
savedSectionOrder, pinnedHiddenBundleIDs, and pinnedAlwaysHiddenBundleIDs using
their typed Defaults.Key cases from MenuBarItemManager instead of raw string
literals. Ensure the corresponding MenuBarItemManager writers and these readers
share the same compile-time key definitions.
In `@ThawTests/MenuBar/Items/CapturedImageScaleTests.swift`:
- Around line 118-135: Update resolvedScaleRestoresPointSize() to declare
throws, then replace try? `#require`(scale) with direct try `#require`(scale) so a
nil resolved scale fails with the framework’s clear requirement error instead of
falling through to the fallback arithmetic.
In `@ThawTests/MenuBar/Items/MenuBarTestFixturesTests.swift`:
- Around line 20-26: Update appItemTagBuildsExpectedNamespaceAndTitle to assert
tag.namespace directly against the expected Namespace case/value instead of
comparing String(describing: tag.namespace). Preserve the existing assertions
for title, instanceIndex, and windowID.
In `@ThawTests/MenuBar/Section/MenuBarSectionNameTests.swift`:
- Around line 90-111: Update localizedVisible, localizedHidden, and
localizedAlwaysHidden to compare each returned LocalizedStringKey directly with
the corresponding LocalizedStringKey literal using Equatable. Remove the
misleading String(describing:) comment and comparisons, while preserving the
existing expected keys.
In `@ThawTests/Settings/Models/AdvancedSettingsTests.swift`:
- Around line 46-80: Update AdvancedSettings to accept an injectable
NotificationCenter, matching the existing center injection pattern used by
debouncedNotificationTask, and use that center when observing settings changes.
In AdvancedSettingsTests, create a private NotificationCenter per test and pass
it to the model, then post through the same isolated center instead of
NotificationCenter.default so external-change assertions cannot receive
notifications from other suites.
In `@ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift`:
- Around line 47-71: Introduce one reusable display-UUID availability condition
and apply it via .enabled(if:) to resolvedDisplayUsesExplicitConfiguration and
resolvedDisplayWithoutOverrideFallsBackToGlobalConfiguration in
ThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swift lines
47-71, removing the try `#require` and early return; apply the same condition to
typedLookupsMatchTheConfiguration and connectedDisplaysAreWellFormed in
ThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swift lines
130-149 and 242-254, removing their early-return or vacuous-loop gating so
headless environments report skipped tests consistently.
In `@ThawTests/Settings/Models/GeneralSettingsTests.swift`:
- Around line 27-35: Update GeneralSettingsTests to use the injectable Defaults
store: first confirm GeneralSettings reads and writes through Defaults, then
wrap every test with withScratchDefaults. Remove the per-key snapshot and
restore logic and revise the related documentation to describe the scratch-store
isolation instead.
In `@ThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swift`:
- Around line 38-40: Strengthen the clearing assertion in the hotkey persistence
test by first unwrapping the dictionary returned by Defaults.dictionary(forKey:
.hotkeys), then assert that action.rawValue is absent from the non-optional
dictionary. Avoid optional chaining so a missing dictionary cannot make the
assertion pass.
In `@ThawTests/Settings/Models/ProfileDecodingDefaultsTests.swift`:
- Around line 590-608: Update the documentation above
duplicateIdentifierResolvesToOneSection by either naming the specific report
file or removing the sentence referring to an accompanying report. Keep the
existing explanation about Dictionary iteration order and the test’s assertion
unchanged.
- Around line 732-748: Update applyPreservesLastCustomIceIcon to set
snapshot.iceIcon to a .custom icon before applying it, while retaining a
distinct remembered value in snapshot.lastCustomIceIcon. Use the existing
custom-icon setup from applyGeneralSettingsCustomIconKeepsLastCustom so
iceIcon’s didSet mirror executes and the test distinguishes the assignment
order.
In `@ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift`:
- Around line 31-36: Update the affected doc comments in the
ProfileLayoutLogReplayTests suite to remove the obsolete XCTest “test” prefix
and reference the migrated methods withoutExclusionTheOrphanWouldBeUnmanaged,
buggyCycleDoesNotPlanMoveForUnresolvedOrphan,
loggedDesiredVisibleIsUsedInsteadOfInference, and
displayReconnectNegativeBudgetYieldsNoOverflow.
In `@ThawTests/Settings/URI/SettingsURIHandlerApplyTests.swift`:
- Around line 29-53: Move the shared notifications(named:during:) helper and
NotificationBox class into ThawTests/Support/, then remove both private local
copies from SettingsURIHandlerApplyTests and SettingsURIHandlerCoverageTests.
Preserve their existing behavior and signatures so both test suites reuse the
shared implementation.
In `@ThawTests/Settings/URI/SettingsURIParserTests.swift`:
- Around line 163-177: Update parsingIsTotal to require every test argument to
produce a URL instead of returning silently when URL(string:) is nil. Reuse the
file’s existing helper that enforces URL creation, then pass the resulting URL
to SettingsURIParser.parse.
In `@ThawTests/Shared/DiagnosticLoggerFileTests.swift`:
- Around line 311-324: Update latestLogFileIsSelfConsistent to remove the
cross-read assertion comparing logger.hasLogFiles with latest, since both
accessors independently scan the shared log directory. Retain the existing
latestLogFile shape checks, including the .log extension and logDirectory
parent-path validation.
In `@ThawTests/Support/ProfileTestFixtures.swift`:
- Around line 111-135: The profile test fixtures duplicate encoder and
manifest-seeding logic across three files; centralize it to prevent
configuration drift. In ThawTests/Support/ProfileTestFixtures.swift lines
111-135, add shared makeProfileEncoder() and extend seedManifest with
omittingFilesFor: Set<UUID> = [], skipping those profile files while still
writing the manifest. In
ThawTests/Settings/Models/ProfileManagerPersistenceTests.swift lines 585-624,
remove private seedManifest and makeEncoder, use the shared helpers, and
preserve withManager and writeBundle. In
ThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swift lines
120-129, replace local JSONEncoder setup in writeProfile with
makeProfileEncoder().
In `@ThawTests/Support/ScratchDefaults.swift`:
- Around line 45-53: Update acquire() to handle task cancellation while waiting
for the lock by wrapping the waiter continuation in withTaskCancellationHandler
and ensuring cancellation resumes or removes the suspended waiter safely.
Preserve the existing immediate-acquisition behavior and prevent cancelled test
tasks from remaining suspended indefinitely.
- Around line 133-143: Unify the synchronization used by the synchronous and
asynchronous ScratchDefaults overloads so they cannot interleave while modifying
the global Defaults.store. Update scratchDefaultsLock and scratchDefaultsMutex
usage so both overloads share one mutual-exclusion mechanism, preserving each
overload’s existing setup, restoration, and cleanup behavior without allowing
one caller to restore another caller’s deleted store.
In `@ThawTests/Utilities/CGImageAnalysisTests.swift`:
- Around line 197-210: Update fallbackAgreesWithFastPath to assert the expected
transparency value for each opaque input in addition to comparing fallback and
normal results. Use opaque to derive the absolute expectation, preserving the
existing agreement assertion.
In `@ThawTests/Utilities/CGImageDetachedCopyTests.swift`:
- Around line 45-63: Update pixelData(of:) to create and use the CGContext
inside data.withUnsafeMutableBytes, passing the buffer pointer as the context
data and performing context.draw within the same closure. Preserve the existing
bitmap configuration, context-creation failure handling, and returned pixel
data.
---
Outside diff comments:
In `@Thaw/Events/HIDEventManager.swift`:
- Around line 1085-1091: Update the sleep guard inside the Task closure in
HIDEventManager to use the Swift-required operator order, placing try? before
await: guard (try? await Task.sleep(...)) != nil. Preserve the existing
cancellation behavior and 250-millisecond delay.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 202-207: Change isDegradedIdentityEnrichmentEnabled from a cached
static let to a computed static property that reads Defaults.store.bool(forKey:
"EnableDegradedItemAXEnrichment") on every access, preserving the existing key
and nonisolated behavior.
- Around line 7051-7057: Update the post-arm abort handling in the functions
containing the unresolved-sourcePID, open-menu, missing-control-item, and
post-Phase-1 control-loss paths to call restoreProfileStateAfterCancelledApply
with the same apply token guard instead of clearProfileState. Preserve
clearProfileState for successful completion, and ensure each cancellation
restores all armed profile state and releases priorProfileApplySnapshot without
persisting changes.
In `@Thaw/Settings/Models/AdvancedSettings.swift`:
- Around line 270-279: Correct the documentation for
sanitizedSearchSectionOrder(from:) to state that empty or entirely unrecognized
input falls back to MenuBarSection.Name.allCases in declaration order, not
Defaults.DefaultValue.searchSectionOrder. Keep the implementation unchanged
unless intentionally aligning its behavior with the documented default.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4245c0bb-afa2-49e9-8370-65b05294d623
📒 Files selected for processing (243)
.github/workflows/ci.yml.gitignore.swiftformatMenuBarItemService/Listener.swiftMenuBarItemService/SourcePIDCache.swiftMenuBarItemService/main.swiftShared/Bridging/Shims.swiftShared/Utilities/DiagnosticLogger.swiftShared/Utilities/MarkerPairResolver.swiftThaw/Events/EventTap.swiftThaw/Events/HIDEventManager.swiftThaw/Events/RunLoopLocalEventMonitor.swiftThaw/Hotkeys/Hotkey.swiftThaw/Hotkeys/HotkeyRegistry.swiftThaw/Main/AppState.swiftThaw/Main/Updates.swiftThaw/MenuBar/Appearance/Configurations/MenuBarAppearanceConfigurationV2.swiftThaw/MenuBar/Appearance/Configurations/MenuBarShapes.swiftThaw/MenuBar/Appearance/MissionControlDetector.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/LayoutBar/LayoutBarPaddingView.swiftThaw/MenuBar/MenuBarItems/AXIdentityCatalog.swiftThaw/MenuBar/MenuBarItems/AXItemActivator.swiftThaw/MenuBar/MenuBarItems/ClickReactionVerifier.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItem+Enumeration.swiftThaw/MenuBar/MenuBarItems/MenuBarItem.swiftThaw/MenuBar/MenuBarItems/MenuBarItemFailureLedger.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swiftThaw/MenuBar/MenuBarItems/MenuBarItemTag.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/MenuBarSection.swiftThaw/MenuBar/MenuBarSectionLayout.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Onboarding/Tour/OnboardingTour.swiftThaw/Onboarding/Tour/TourMockups.swiftThaw/Onboarding/Tour/WelcomeMockup.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/FocusFilterIntent.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/UI/Views/CustomTooltip.swiftThaw/Utilities/ConcurrencyHelpers.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/Extensions.swiftThaw/Utilities/ScreenCapture.swiftThaw/Utilities/SettingsURIHandler.swiftThaw/Utilities/SettingsURIParser.swiftThawTests/AdvancedSettingsTests.swiftThawTests/AutomationSettingsTests.swiftThawTests/CaptureBoundsValidationTests.swiftThawTests/CrossCutting/CoverageSweep1Tests.swiftThawTests/CrossCutting/CoverageSweep2Tests.swiftThawTests/CrossCutting/CoverageSweep3Tests.swiftThawTests/CrossCutting/CoverageSweep4Tests.swiftThawTests/CrossCutting/CoverageSweep5Tests.swiftThawTests/DefaultsKeyTests.swiftThawTests/DiagnosticLoggerTests.swiftThawTests/DisplayIceBarConfigurationTests.swiftThawTests/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/DisplaySettingsManagerMutationTests.swiftThawTests/DisplaySettingsManagerSpacingGateTests.swiftThawTests/ExtensionsTests.swiftThawTests/GeneralSettingsSnapshotTests.swiftThawTests/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyActionTests.swiftThawTests/Hotkeys/HotkeyLifecycleTests.swiftThawTests/Hotkeys/KeyCodeTests.swiftThawTests/Hotkeys/KeyCombinationTests.swiftThawTests/Hotkeys/ModifiersTests.swiftThawTests/HotkeysSettingsPersistenceTests.swiftThawTests/IceBarLocationTests.swiftThawTests/IceSettingsImporterTests.swiftThawTests/LayoutResetErrorTests.swiftThawTests/MenuBar/Appearance/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBar/Appearance/MenuBarShapesTests.swiftThawTests/MenuBar/Appearance/MenuBarTintKindTests.swiftThawTests/MenuBar/Appearance/MissionControlDetectorTests.swiftThawTests/MenuBar/ControlItem/ControlItemImageTests.swiftThawTests/MenuBar/ControlItem/ControlItemOcclusionTests.swiftThawTests/MenuBar/ControlItem/ControlItemRecoveryTests.swiftThawTests/MenuBar/IceBar/IceBarLocationTests.swiftThawTests/MenuBar/Items/AXIdentityCatalogTests.swiftThawTests/MenuBar/Items/AXItemActivatorTests.swiftThawTests/MenuBar/Items/AnchorDestinationTests.swiftThawTests/MenuBar/Items/BadgeIndexTests.swiftThawTests/MenuBar/Items/CaptureBoundsValidationTests.swiftThawTests/MenuBar/Items/CapturedImageScaleTests.swiftThawTests/MenuBar/Items/ClickReactionVerifierTests.swiftThawTests/MenuBar/Items/ControlCenterHostedMatchLogReplayTests.swiftThawTests/MenuBar/Items/DisplaySpreadGateTests.swiftThawTests/MenuBar/Items/DivergencePersistenceGateTests.swiftThawTests/MenuBar/Items/FlattenCurrentSectionsTests.swiftThawTests/MenuBar/Items/GhostControlItemWindowTests.swiftThawTests/MenuBar/Items/HiddenDragFailureClassificationTests.swiftThawTests/MenuBar/Items/MenuBarItemFailureLedgerTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheDisplayResolutionTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheGatingTests.swiftThawTests/MenuBar/Items/MenuBarItemImageCacheObservationTests.swiftThawTests/MenuBar/Items/MenuBarItemManagerRearmTests.swiftThawTests/MenuBar/Items/MenuBarItemOrderingTests.swiftThawTests/MenuBar/Items/MenuBarItemServiceTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCodingTests.swiftThawTests/MenuBar/Items/MenuBarItemTagTests.swiftThawTests/MenuBar/Items/MenuBarItemValueTests.swiftThawTests/MenuBar/Items/MenuBarTestFixturesTests.swiftThawTests/MenuBar/Items/MidSectionTransitionTests.swiftThawTests/MenuBar/Items/MoveEventCoordinatesTests.swiftThawTests/MenuBar/Items/MoveFailureBackoffTests.swiftThawTests/MenuBar/Items/MoveMenuGuardTests.swiftThawTests/MenuBar/Items/NewItemsPlacementTests.swiftThawTests/MenuBar/Items/PartitionUnmanagedUIDsTests.swiftThawTests/MenuBar/Items/PendingRehideTagIdentifiersTests.swiftThawTests/MenuBar/Items/SavedLayoutSectionLookupTests.swiftThawTests/MenuBar/Items/SavedPositionLookupTests.swiftThawTests/MenuBar/Items/SectionGeometryGateTests.swiftThawTests/MenuBar/Items/SelectWindowForBatchScanTests.swiftThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swiftThawTests/MenuBar/Items/SourcePIDResolutionGateTests.swiftThawTests/MenuBar/Items/WindowIDsChangedGateTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerTests.swiftThawTests/MenuBar/Layout/LayoutReconcilerUnmanagedPlacementTests.swiftThawTests/MenuBar/Layout/LayoutResetErrorTests.swiftThawTests/MenuBar/Layout/LayoutResetReentrancyTests.swiftThawTests/MenuBar/Layout/NotchOverflowRevealTests.swiftThawTests/MenuBar/Layout/PlanFullSortSequenceTests.swiftThawTests/MenuBar/Layout/PlanLCSMoveSequenceTests.swiftThawTests/MenuBar/Layout/PlanLeftmostMoveTests.swiftThawTests/MenuBar/Layout/PlanNotchOverflowTests.swiftThawTests/MenuBar/Layout/PlanPendingMoveTests.swiftThawTests/MenuBar/Layout/PlanSectionOrderTests.swiftThawTests/MenuBar/Layout/PlanUnmanagedPlacementTests.swiftThawTests/MenuBar/Section/MenuBarSectionGeometryTests.swiftThawTests/MenuBar/Section/MenuBarSectionNameTests.swiftThawTests/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBarItemServiceTests.swiftThawTests/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBarItemTagTests.swiftThawTests/MenuBarSectionNameTests.swiftThawTests/MenuBarShapesTests.swiftThawTests/MenuBarTestFixturesTests.swiftThawTests/ModifiersTests.swiftThawTests/MoveFailureBackoffTests.swiftThawTests/Onboarding/OnboardingMockupsTests.swiftThawTests/Onboarding/OnboardingSlideTests.swiftThawTests/OnboardingMockupsTests.swiftThawTests/OnboardingSlideTests.swiftThawTests/PermissionTests.swiftThawTests/Permissions/PermissionTests.swiftThawTests/ProfileEntityTests.swiftThawTests/ProfileExportBundleTests.swiftThawTests/ProfileManagerDeleteTests.swiftThawTests/ProfileManagerRearmGateTests.swiftThawTests/ProfileTests.swiftThawTests/RehideStrategyTests.swiftThawTests/SectionDividerStyleTests.swiftThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/AutomationSettingsTests.swiftThawTests/Settings/Models/DisplayIceBarConfigurationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/Settings/Models/DisplaySettingsManagerLookupTests.swiftThawTests/Settings/Models/DisplaySettingsManagerMutationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerSpacingGateTests.swiftThawTests/Settings/Models/DisplaySettingsManagerURINotificationTests.swiftThawTests/Settings/Models/ExternalSettingsChangeTests.swiftThawTests/Settings/Models/FocusFilterIntentTests.swiftThawTests/Settings/Models/GeneralSettingsSnapshotTests.swiftThawTests/Settings/Models/GeneralSettingsTests.swiftThawTests/Settings/Models/HookScriptTests.swiftThawTests/Settings/Models/HotkeysSettingsPersistenceTests.swiftThawTests/Settings/Models/ProfileApplyInFlightFlagTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/Models/ProfileEntityTests.swiftThawTests/Settings/Models/ProfileExportBundleTests.swiftThawTests/Settings/Models/ProfileLayoutLogReplayTests.swiftThawTests/Settings/Models/ProfileManagerCRUDTests.swiftThawTests/Settings/Models/ProfileManagerDeepTests.swiftThawTests/Settings/Models/ProfileManagerDeleteTests.swiftThawTests/Settings/Models/ProfileManagerObservationTasksTests.swiftThawTests/Settings/Models/ProfileManagerPersistenceTests.swiftThawTests/Settings/Models/ProfileManagerRearmGateTests.swiftThawTests/Settings/Models/ProfileManagerUpdateRearmIntegrationTests.swiftThawTests/Settings/Models/ProfileTests.swiftThawTests/Settings/Models/RehideStrategyTests.swiftThawTests/Settings/Models/SectionDividerStyleTests.swiftThawTests/Settings/Models/SettingsResetterTests.swiftThawTests/Settings/Search/SearchIndexTests.swiftThawTests/Settings/Search/SearchModelTests.swiftThawTests/Settings/Search/SettingsSearchNavigationTests.swiftThawTests/Settings/URI/IntegrationURIContractTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Settings/URI/SettingsURIHandlerGetTests.swiftThawTests/Settings/URI/SettingsURIHandlerTailTests.swiftThawTests/Settings/URI/SettingsURIHandlerTests.swiftThawTests/Settings/URI/SettingsURIParserTests.swiftThawTests/SettingsResetterTests.swiftThawTests/SettingsURIHandlerApplyTests.swiftThawTests/SettingsURIHandlerGetTests.swiftThawTests/SettingsURIHandlerTests.swiftThawTests/Shared/CodeSigningInfoTests.swiftThawTests/Shared/DiagnosticLoggerFileTests.swiftThawTests/Shared/DiagnosticLoggerTests.swiftThawTests/Shared/MarkerPairResolverTests.swiftThawTests/Shared/SharedExtensionsTests.swiftThawTests/Shared/WindowInfoDecodingTests.swiftThawTests/Shared/WindowInfoDerivedTests.swiftThawTests/Shared/WindowInfoTests.swiftThawTests/SharedExtensionsTests.swiftThawTests/Support/GraphicsTestFixtures.swiftThawTests/Support/MenuBarTestFixtures.swiftThawTests/Support/ProfileTestFixtures.swiftThawTests/Support/ScratchDefaults.swiftThawTests/Utilities/CGImageAnalysisTests.swiftThawTests/Utilities/CGImageDetachedCopyTests.swiftThawTests/Utilities/ConcurrencyHelpersTests.swiftThawTests/Utilities/CustomTooltipPanelTests.swiftThawTests/Utilities/DefaultsKeyTests.swiftThawTests/Utilities/ExtensionsCoverageTests.swiftThawTests/Utilities/ExtensionsGraphicsTests.swiftThawTests/Utilities/ExtensionsRemainderTests.swiftThawTests/Utilities/ExtensionsTests.swiftThawTests/Utilities/HookRunnerTests.swiftThawTests/Utilities/IceSettingsImporterTailTests.swiftThawTests/Utilities/IceSettingsImporterTests.swiftThawTests/Utilities/LocalizedErrorWrapperTests.swiftThawTests/Utilities/MigrationTests.swiftThawTests/Utilities/MouseMovedThrottleTests.swiftThawTests/Utilities/ObjectStorageTests.swiftThawTests/Utilities/PredicatesTests.swiftThawTests/Utilities/ShareableContentCacheTests.swiftThawTests/Utilities/SimpleSemaphoreTests.swiftThawTests/WindowInfoTests.swiftdocs/ASSURANCE_CASE.mdscripts/coverage-report.pyscripts/swiftlint-inputs.xcfilelistsonar-project.properties
💤 Files with no reviewable changes (28)
- ThawTests/DiagnosticLoggerTests.swift
- ThawTests/GeneralSettingsSnapshotTests.swift
- ThawTests/ModifiersTests.swift
- ThawTests/IceBarLocationTests.swift
- ThawTests/LayoutResetErrorTests.swift
- ThawTests/ExtensionsTests.swift
- ThawTests/MenuBarTestFixturesTests.swift
- ThawTests/AdvancedSettingsTests.swift
- ThawTests/DisplayIceBarConfigurationTests.swift
- ThawTests/HotkeysSettingsPersistenceTests.swift
- ThawTests/IceSettingsImporterTests.swift
- ThawTests/MoveFailureBackoffTests.swift
- ThawTests/DisplaySettingsManagerGlobalFallbackTests.swift
- ThawTests/MenuBarSectionNameTests.swift
- ThawTests/DisplaySettingsManagerMutationTests.swift
- ThawTests/AutomationSettingsTests.swift
- ThawTests/MenuBarItemTagCanonicalizationTests.swift
- Thaw/MenuBar/Appearance/MissionControlDetector.swift
- ThawTests/DefaultsKeyTests.swift
- ThawTests/DisplaySettingsManagerSpacingGateTests.swift
- ThawTests/CaptureBoundsValidationTests.swift
- Thaw/MenuBar/MenuBarSection.swift
- ThawTests/MenuBarShapesTests.swift
- ThawTests/MenuBarItemTagTests.swift
- ThawTests/MenuBar/Items/GhostControlItemWindowTests.swift
- ThawTests/MenuBarItemServiceTests.swift
- ThawTests/MenuBarAppearanceConfigurationTests.swift
- ThawTests/HotkeyActionTests.swift
🛑 Comments failed to post (6)
ThawTests/MenuBar/Items/CapturedImageScaleTests.swift (1)
118-135: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use
try#require`` instead oftry?for a clearer failure message.
resolvedScaleRestoresPointSize()is not markedthrows, sotry?#require(scale)converts a#requirefailure into anOptional. Ifscaleisnil, the test does not fail with a clear "required value is nil" message. It instead falls through toresolved ?? 1and fails later with a confusing arithmetic mismatch.Mark the test
throwsand calltry#require(scale)directly.♻️ Proposed fix
- `@Test`("The resolved scale gives the item back its true point size") - func resolvedScaleRestoresPointSize() { + `@Test`("The resolved scale gives the item back its true point size") + func resolvedScaleRestoresPointSize() throws { // The property that actually matters downstream: CapturedImage // divides pixels by scale to get points, and the layout bar sizes // its rows from that. Under the old behavior this produced 1850pt // instead of 925pt — the doubled row height in the screenshot. let boundsWidth: CGFloat = 925 let pixelWidth = 1850 let scale = MenuBarItemImageCache.resolvedScale( imagePixelWidth: pixelWidth, boundsWidth: boundsWidth, expected: 1 ) - let resolved = try? `#require`(scale) - `#expect`(CGFloat(pixelWidth) / (resolved ?? 1) == boundsWidth) + let resolved = try `#require`(scale) + `#expect`(CGFloat(pixelWidth) / resolved == boundsWidth) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test`("The resolved scale gives the item back its true point size") func resolvedScaleRestoresPointSize() throws { // The property that actually matters downstream: CapturedImage // divides pixels by scale to get points, and the layout bar sizes // its rows from that. Under the old behavior this produced 1850pt // instead of 925pt — the doubled row height in the screenshot. let boundsWidth: CGFloat = 925 let pixelWidth = 1850 let scale = MenuBarItemImageCache.resolvedScale( imagePixelWidth: pixelWidth, boundsWidth: boundsWidth, expected: 1 ) let resolved = try `#require`(scale) `#expect`(CGFloat(pixelWidth) / resolved == boundsWidth) }🤖 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 `@ThawTests/MenuBar/Items/CapturedImageScaleTests.swift` around lines 118 - 135, Update resolvedScaleRestoresPointSize() to declare throws, then replace try? `#require`(scale) with direct try `#require`(scale) so a nil resolved scale fails with the framework’s clear requirement error instead of falling through to the fallback arithmetic.ThawTests/Settings/Models/GeneralSettingsTests.swift (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the injectable Defaults store instead of snapshotting real
UserDefaults.The doc comment states that
Defaultsis hardcoded to.standard. This PR addsDefaults.storeand awithScratchDefaultshelper, and sibling suites such asDisplaySettingsManagerURINotificationTestsalready use it. With a scratch store, this suite would not write into the developer's own settings, and the cross-suite interference documented at Lines 106-110 and Lines 308-311 would disappear.Confirm that
GeneralSettingsreads and writes only throughDefaults, then wrap each test inwithScratchDefaultsand drop the snapshot/restore code.#!/bin/bash # Confirm the injectable store and the scratch helper, then check GeneralSettings' storage path. fd -i 'ScratchDefaults\.swift$' . fd -i 'ScratchDefaults\.swift$' . --exec cat -n fd -i '^Defaults\.swift$' . --exec rg -n 'static (var|let) store|withScratchDefaults' fd -i '^GeneralSettings\.swift$' . --exec rg -n 'UserDefaults|Defaults\.'🤖 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 `@ThawTests/Settings/Models/GeneralSettingsTests.swift` around lines 27 - 35, Update GeneralSettingsTests to use the injectable Defaults store: first confirm GeneralSettings reads and writes through Defaults, then wrap every test with withScratchDefaults. Remove the per-key snapshot and restore logic and revise the related documentation to describe the scratch-store isolation instead.ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift (1)
31-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the doc comments to the migrated test names.
These comments still name the XCTest methods with the
testprefix:testWithoutExclusionTheOrphanWouldBeUnmanaged,testBuggyCycleDoesNotPlanMoveForUnresolvedOrphan,testLoggedDesiredVisibleIsUsedInsteadOfInference, andtestDisplayReconnectNegativeBudgetYieldsNoOverflow. The suite now declareswithoutExclusionTheOrphanWouldBeUnmanaged,buggyCycleDoesNotPlanMoveForUnresolvedOrphan,loggedDesiredVisibleIsUsedInsteadOfInference, anddisplayReconnectNegativeBudgetYieldsNoOverflow. A reader who searches for the documented names finds nothing.📝 Proposed fix
-/// unmanaged set inside the live partitioner. Two tests pin it down: -/// testWithoutExclusionTheOrphanWouldBeUnmanaged documents the bug mechanism -/// (with no exclusion the orphan is classified unmanaged, matching the field -/// log), and testBuggyCycleDoesNotPlanMoveForUnresolvedOrphan is the regression -/// lock that fails before the fix and passes after it. +/// unmanaged set inside the live partitioner. Two tests pin it down: +/// withoutExclusionTheOrphanWouldBeUnmanaged documents the bug mechanism +/// (with no exclusion the orphan is classified unmanaged, matching the field +/// log), and buggyCycleDoesNotPlanMoveForUnresolvedOrphan is the regression +/// lock that fails before the fix and passes after it.- /// desiredVisible (older captures do not log it; see - /// testLoggedDesiredVisibleIsUsedInsteadOfInference) or the two - /// notch-overflow lines (see testDisplayReconnectNegativeBudgetYieldsNoOverflow). + /// desiredVisible (older captures do not log it; see + /// loggedDesiredVisibleIsUsedInsteadOfInference) or the two + /// notch-overflow lines (see displayReconnectNegativeBudgetYieldsNoOverflow).Also applies to: 69-75
🤖 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 `@ThawTests/Settings/Models/ProfileLayoutLogReplayTests.swift` around lines 31 - 36, Update the affected doc comments in the ProfileLayoutLogReplayTests suite to remove the obsolete XCTest “test” prefix and reference the migrated methods withoutExclusionTheOrphanWouldBeUnmanaged, buggyCycleDoesNotPlanMoveForUnresolvedOrphan, loggedDesiredVisibleIsUsedInsteadOfInference, and displayReconnectNegativeBudgetYieldsNoOverflow.ThawTests/Settings/URI/SettingsURIParserTests.swift (1)
163-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the totality test fail when an input is not a URL.
parsingIsTotalreturns silently whenURL(string:)returns nil. That case then passes without calling the parser, so an input that stops being a valid URL becomes a dead case with no signal. The file already has a helper that requires a URL; use the same rule here.💚 Proposed fix
- func parsingIsTotal(uri: String) { - guard let url = URL(string: uri) else { return } - _ = SettingsURIParser.parse(url) + func parsingIsTotal(uri: String) throws { + let url = try `#require`(URL(string: uri), "Could not build a URL from \(uri)") + _ = SettingsURIParser.parse(url) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// The parser is total: any URL yields a request and nothing traps. `@Test`("Parsing is total over awkward input", arguments: [ "thaw://set?key=a&value=b&key=c", "thaw://set?key=%00&value=%FF", "thaw://get?callback=javascript:alert(1)", "thaw://toggle?key=" + String(repeating: "x", count: 8192), "thaw://set?=&=&=", "thaw://%20", "thaw://get?key=a#fragment", ]) func parsingIsTotal(uri: String) throws { let url = try `#require`(URL(string: uri), "Could not build a URL from \(uri)") _ = SettingsURIParser.parse(url) } }🤖 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 `@ThawTests/Settings/URI/SettingsURIParserTests.swift` around lines 163 - 177, Update parsingIsTotal to require every test argument to produce a URL instead of returning silently when URL(string:) is nil. Reuse the file’s existing helper that enforces URL creation, then pass the resulting URL to SettingsURIParser.parse.ThawTests/Shared/DiagnosticLoggerFileTests.swift (1)
311-324: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The
hasLogFiles == (latest != nil)assertion reads the shared real log directory twice and can flake.
latestis read at Line 317 andhasLogFilesat Line 319. Both accessors scan~/Library/Logs/Thaw, which is not owned by this suite. Another suite that enablesDiagnosticLogger, or the developer's own running app, can create or rotate a file between the two reads. The equality then fails for a reason unrelated to the accessors.Assert the shape of
latestLogFileand drop the cross-read equality, or derive both values from one directory listing.🛠️ Proposed fix
- let logger = DiagnosticLogger.shared - let latest = logger.latestLogFile - - `#expect`(logger.hasLogFiles == (latest != nil)) - if let latest { + let logger = DiagnosticLogger.shared + + if let latest = logger.latestLogFile { + // A file was found, so the directory is non-empty by definition. + `#expect`(logger.hasLogFiles) `#expect`(latest.pathExtension == "log") `#expect`(latest.deletingLastPathComponent().path == logger.logDirectory.path) }🤖 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 `@ThawTests/Shared/DiagnosticLoggerFileTests.swift` around lines 311 - 324, Update latestLogFileIsSelfConsistent to remove the cross-read assertion comparing logger.hasLogFiles with latest, since both accessors independently scan the shared log directory. Retain the existing latestLogFile shape checks, including the .log extension and logDirectory parent-path validation.ThawTests/Utilities/CGImageAnalysisTests.swift (1)
197-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin the expected transparency value, not only the agreement between the two paths.
The assertion compares the fallback result with the fast-path result. If both routes regress in the same direction, the test still passes. Add the absolute expectation so the case fails when either route is wrong.
♻️ Proposed change
- `#expect`(fallback.isTransparent() == normal.isTransparent()) + `#expect`(fallback.isTransparent() == !opaque) + `#expect`(normal.isTransparent() == !opaque) + `#expect`(fallback.isTransparent() == normal.isTransparent())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test`("The fallback path agrees with the fast path", arguments: [true, false]) func fallbackAgreesWithFastPath(_ opaque: Bool) throws { // An image with no explicit byte order is outside the fast path, so // this exercises the TransparencyContext fallback. let fallback = try makeDefaultByteOrderImage(width: 8, height: 8, opaque: opaque) let normal = try makeCanvas(width: 8, height: 8) { context in if opaque { context.setFillColor(red: 1, green: 0, blue: 0, alpha: 1) context.fill(CGRect(x: 0, y: 0, width: 8, height: 8)) } } `#expect`(fallback.isTransparent() == !opaque) `#expect`(normal.isTransparent() == !opaque) `#expect`(fallback.isTransparent() == normal.isTransparent()) }🤖 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 `@ThawTests/Utilities/CGImageAnalysisTests.swift` around lines 197 - 210, Update fallbackAgreesWithFastPath to assert the expected transparency value for each opaque input in addition to comparing fallback and normal results. Use opaque to derive the absolute expectation, preserving the existing agreement assertion.
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.
Summary
Migrate the remaining XCTest suite to Swift Testing, raise unit coverage, and land the small product fixes/refactors that the new test seams unlocked.
Scope: Large by design — the XCTest → Swift Testing migration touches the whole
ThawTests/tree (reorg + ~79 file migration) and cannot be usefully split from the coverage sweep or the Defaults / MenuBar / profiles seams those tests require. Product fixes in this PR are minimal and gated on those seams.Closes: N/A
PR Type
Describe what this change does (not the linked issue’s request kind). Bug reports use
bugon issues; bug fixes usefixon PRs.If you tick Feature or Refactor and touch more than ~20 files, please mention why this can’t be split.
Area
Product surfaces (optional when the change is not about the app UI). Path-based labeling also applies.
Use PR Type for what changed (
CI/CD,Documentation,Other/ chore, etc.).Use
opsfor where when it is repo operations: CI, release, GitHub hygiene, scripts, lint/sonar config — not a product surface.menubar
icebar
layout
appearance
settings
onboarding
permissions
profiles
hotkeys
updates
ops
Does this PR introduce a breaking change?
What is the new behavior?
ThawTests/moved to Swift Testing (import Testing); 0 XCTest imports left. Suites reorganized into folders mirroring the source tree.scripts/coverage-report.pyuses Sonar’s denominator.MenuBarSection/MenuBarItemsplit along testable boundaries; SettingsResetter tests no longer wipe the real defaults domain.Algorithms.uniqued()for order-preserving dedups.docs/ASSURANCE_CASE.md;.gitignorefor.swiftpm-overrides; sonar property tweaks; swiftformat hygiene (including not stripping stored raw values).PR Checklist
swiftformat .to keep the code style consistent.xcodebuild test …orswift test --package-path MenuBarModel.developmentbranch.Test commands run:
Known limitations / follow-ups
development— may need a rebase before merge.Other information
test/swift-testing-migration-and-coverage(~21 commits ahead ofdevelopment)Summary by CodeRabbit
New Features
Bug Fixes
Documentation