Skip to content

Audit remediation, phases 0-4 [3/4] - #1505

Open
theboringhumane wants to merge 10 commits into
stack/02-compact-modefrom
stack/03-audit-remediation
Open

Audit remediation, phases 0-4 [3/4]#1505
theboringhumane wants to merge 10 commits into
stack/02-compact-modefrom
stack/03-audit-remediation

Conversation

@theboringhumane

@theboringhumane theboringhumane commented Aug 29, 2026

Copy link
Copy Markdown
Member

Stack 3 of 4. Based on #1504; base for #1506. Split out of #1496 for reviewability.

Summary

  • Phase 0: crash fixes, dead-code purge (incl. ProgressIndicator.swift), OSS compliance
  • Phase 1: background polling killed, OSD hot paths fixed
  • Phase 2: coordinator retain cycle broken, architecture decoupled
  • Phase 3: naming, dedup, conventions, structured logging (Log.swift); Live activities/LiveActivities/
  • Phase 4: community files, tests, lint, security docs; app-target DEVELOPMENT_TEAM restored for automatic signing

Large but mechanical; the feature stacks (#1503, #1504) are refactored here, not changed behaviorally.

Review round 1 (Alexander5015) — all 12 addressed

  • SECURITY.md: supported-versions table removed
  • .gitignore: reverted wholesale (the cleanup over-reached and let an xcscheme slip in); scheme file untracked, *.xcscheme ignored again
  • .swiftlint.yml: narrative comments trimmed; SwiftLint CI added (.github/workflows/swiftlint.yml, PRs to dev + stack/**)
  • dependabot.yml: swift ecosystem restored — the "Dependabot can't read the manifest" claim was wrong (Bump github.com/sparkle-project/sparkle from 2.9.4 to 2.9.5 #1434 proves it works)
  • NotchHomeView slider: interval 0.5s → 0.2s (reviewer cap) with corrected rationale comment
  • WebcamPreviewWebcamView (View-suffix convention, matches filename)
  • SystemEventIndicatorModifier: narrating comment removed
  • CODEOWNERS: kept per repo-owner decision — routing, not territory; reply on the thread, happy to reshape the mapping

Notes

Test plan

@Alexander5015 Alexander5015 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had a chance to read everything because the diff is large, but here is a starting place

Comment thread SECURITY.md Outdated
Only the latest release (and the `main` branch) receives security fixes.
Beta builds on the `dev` branch are development snapshots.

| Version | Supported |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table should be removed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 1f01e1e — table is gone, intro and reporting sections kept.

}

if shouldShowCamera {
CameraPreviewView(webcamManager: webcamManager)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically its best practice to append View to the names of views

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — renamed WebcamPreviewWebcamView so the type matches its file and the View-suffix convention (4060c04); call sites updated. WebcamPreviewLayer (the NSViewRepresentable) is unchanged.

Comment thread .gitignore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for these changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason — the cleanup over-reached, and dropping *.xcscheme is exactly what let the scheme file get committed (your other comment). Reverted .gitignore wholesale to dev's version in 1f01e1e.

Comment thread .swiftlint.yml Outdated
@@ -0,0 +1,85 @@
# SwiftLint configuration for boring.notch
# Run with: swiftlint --config .swiftlint.yml
# CI wiring is tracked as a follow-up (needs a lint-capable runner).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be too hard to add this into CI?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — .github/workflows/swiftlint.yml added in 088f697 (checkout → brew install swiftlint → swiftlint --config .swiftlint.yml, on PRs to dev and stack/**). One note: it uses the standard macos-26 label that cicd.yml's matrix already uses rather than the self-hosted xcode-27 pin — one-word change if you'd rather have it there.

Comment thread .swiftlint.yml Outdated
# Run with: swiftlint --config .swiftlint.yml
# CI wiring is tracked as a follow-up (needs a lint-capable runner).

# Paths to check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 088f697.

Comment thread .github/CODEOWNERS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this, I don't think it aligns well given that these PRs make significant changes to the Area-specific expertise you carved out, which means you should have a great understanding of those areas, and I have made significant changes throughout the codebase, so I also wouldn't consider you more of an expert in every other area either.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair pushback — to be clear on intent: this isn't meant as a territorial carve-out. The area list came from who historically touches what, and the default owner was moved to @theboringhumane deliberately since they've been driving the notification/XPC helper work heavily lately. It's routing, not a claim of sole expertise — your review is still the gate everywhere. Happy to reshape the mapping together (fewer areas, different split) rather than drop the mechanism entirely; what would you prefer?

Comment thread .github/dependabot.yml Outdated
interval: "weekly"
target-branch: "dev"

# NOTE: no "swift" section. SPM packages live in the xcodeproj-embedded

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong because I haven't seen any errors with Dependabot with Swift, where did you see issues with it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nowhere solid — the claim was wrong, and this repo's own history disproves it: the Sparkle 2.9.5 bump (#1434) came from Dependabot's swift ecosystem reading the xcodeproj-embedded manifest just fine. Restored the swift section and dropped the note in 1f01e1e.


private var musicSlider: some View {
TimelineView(.animation(minimumInterval: musicManager.playbackRate > 0 ? 0.1 : nil)) { timeline in
// 0.5s ticks are imperceptible on a minutes-long track (~1px steps)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true. This is not smooth (~1px steps) unless a song is 7–8 minutes long. The maximum interval this should be set to is at most 0.2s.

While the 10 Hz cadence might be a bit aggressive as it is designed to have ~1px steps for songs 1.5 - 2 mins, we don't need to increase it if it isn't a massive drain since the view is only shown briefly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set to 0.2s in 4060c04 — you're right that ~1px only holds for very long tracks. The comment is corrected too: it now states 0.2s as the cap and credits the original 10 Hz design rather than claiming smoothness that isn't there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be committed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untracked in 1f01e1e (git rm --cached, file stays on disk) — and since .gitignore is fully reverted, *.xcscheme is ignored again so it can't sneak back in.

@Binding var icon: String
@Binding var accent: Color?
let showSlider: Bool = false
/// Routed to hardware (volume/brightness) when the bar is dragged —

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is probably unnecessary

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 4060c04.

theboringhumane and others added 7 commits August 30, 2026 11:09
Crash vectors fixed:
- Guard NSScreen.main ?? screens.first in AppDelegate launch path
- Safe fallbacks for global documents/temp dir + bundle ID constants
- NotificationWatcher: CFTypeID-check window attribute before AXUIElement cast
- AudioCaptureManager: degrade to flat bars if FFT setup fails (was fatalError)
- AudioPlayer: guard bundle resource URL and retain sound during playback

Dead code removed (~740 LOC): TestView/FluidSlider, DownloadView,
ProgressIndicator, BoringNotchWindow, BoringStatusMenu, EditPanelView,
sneakPeekEvent + SharedSneakPeek (contained as! Data crash), unused
BatteryActivityManager closures, dead dispatch items, 4 unused keyboard
shortcuts, dead sneak-peek size constants, NotchSpaceManager leftovers.
VisualEffectView extracted into its own file (was hiding in deleted
EditPanelView.swift, used by 5 onboarding views).

OSS compliance & hygiene:
- THIRD_PARTY_LICENSES: add all 12 SPM packages (Apache-2.0 block for
  lottie-spm/swift-collections/swift-syntax, MIT block +7, BSD block +1)
- Fix broken THIRD_PARTY_LICENSES link in README
- mediaremote-adapter: add provenance README (pinned at upstream v0.7.2,
  verified byte-identical script, rebuild/update instructions)
- Rename PR template so GitHub actually renders it; delete 2 stale
  issue templates; fix CONTRIBUTING step numbering; drop Linux devcontainer
- Commit shared boringNotch scheme; clear hardcoded DEVELOPMENT_TEAM
  (CI overrides it); clean up .gitignore (stop ignoring schemes)

Verified: xcodebuild Debug build succeeds for app + XPC helper.
Always-on polling eliminated (~280k wakeups/day):
- NotificationWatcher: adaptive poll cadence — 0.35s while banners are
  live, 2s when idle (was 0.35s forever, ~250k AX tree walks/day)
- XPCHelperClient: replace 3s accessibility-auth XPC poll with
  activation-driven checks + lazy verification (was ~29k RPCs/day)

Volume OSD hot path (~40 sync CoreAudio IPC calls on main per event):
- Cache output-device snapshot (ID + validated property addresses),
  rebuilt only on default-device change
- All CoreAudio IPC moved to a serial background queue; slider writes
  coalesce to 15Hz; UI publishes optimistically
- Bonus fix: volume/mute listeners now re-register after device changes
  (they used to silently stop after the first switch)

Brightness XPC bursts (3-4 RPCs per keypress):
- adjustScreenBrightness now returns the resulting value in one RPC;
  target display UUID cached until display config changes
- Keyboard backlight computes from cached state; repeat deltas accumulate
  so no key press is lost during round-trips
- Protocol copies synced byte-identical between the two targets

Render economy:
- MusicManager: timestampDate only republishes when slider extrapolation
  inputs change (a pause/resume rebases to avoid overshoot)
- YouTube Music: threshold position updates (>0.25s) so per-second ticks
  stop defeating Equatable
- Drop drawingGroup() Metal re-rasterization; MusicSlider TimelineView
  10Hz -> 2Hz (imperceptible on minutes-long tracks)
- MediaKeyInterceptor: single-key CFPreferences read instead of parsing
  NSGlobalDomain per volume key; un-nest redundant Task hops

Security & localization:
- Remove NSAllowsArbitraryLoads (verified loopback is ATS-exempt; all
  app traffic is HTTPS or localhost)
- 20 user-facing NSMenuItem/alert titles in ShelfItemViewModel routed
  through String(localized:) for Crowdin

Verified: Debug build succeeds for app + XPC helper
…itecture

2.1 Manager->Coordinator cycle broken via NotchUIEventBus:
- Managers (Volume/Brightness/KeyboardBacklight/BetterDisplay/Lunar/
  Music/Battery) now publish presentation events; the coordinator is the
  single subscriber and keeps all show/hide policy. Managers are now
  testable without the UI stack.
- Removed nested @ObservedObject coordinator from MusicManager and
  BatteryStatusViewModel (kills whole-tree republishing of both).

2.2 Defaults<->MusicManager init cycle removed:
- New MediaEnvironment owns the NowPlaying availability probe (resolved
  eagerly at launch, persisted for static key defaults).
- Defaults.Keys.mediaController default reads only UserDefaults — can no
  longer re-enter MusicManager's lazy init (latent fatal error).

2.5 XPC contract single-sourced + helper health:
- Both targets now compile the SAME files via a Shared/ synchronized
  group: BoringNotchXPCHelperProtocol.swift + unified JSONLinesPipeHandler
  (the app/helper copies had drifted; helper also lost its dead NSCoder init).
- New XPCHelperError type; client tracks connection health and Settings
  shows a 'Helper Service Unavailable' warning when the helper dies
  instead of features silently degrading.

2.3 AppDelegate god object split (671 -> ~330 lines):
- New NotchWindowManager owns window/view-model/drag-detector lifecycle;
  parallel [UUID] dictionaries unified into one ScreenContext per screen.
- Fixes latent leak: windowScreenDidChange observers are now removed
  before re-registration (used to leak one per window recreation).

2.4 ShelfItemViewModel god object split (1155 -> 182 lines):
- AppKit context-menu construction + dispatch extracted to
  ShelfContextMenu.swift via closure bridge (no singleton back-ref).
- Menu dispatch is now tag-based (ContextMenuAction rawValue) instead of
  matching NSMenuItem.title — which silently broke under any non-English
  locale, and would have broken as soon as titles were localized.
- Menu strings now use the file's existing translated Strings table
  (Shelf.ContextMenu.* keys present in the catalog) instead of literals.
- Removed dead loadOpenWithApps.

2.6 Concurrency hygiene:
- MusicManager is now @mainactor (was publishing from arbitrary tasks).
- WebcamManager: removed @Published-duplicate didSet objectWillChange sends.

Verified: Debug build succeeds for app + XPC helper.
…ging

Naming & layout:
- All 12 Settings view types now match their files (Advanced ->
  AdvancedSettingsView, Charge -> BatterySettingsView, MirrorSettings ->
  WebcamSettingsView, About -> AboutView, etc.)
- Lowercase 'sneakPeek' struct -> SneakPeekState; Type2Name -> osdTypeName;
  BrightnessSymbolString -> brightnessSymbolName; AppIcon()/
  AppIconAsNSImage() -> lowerCamel; cryptic 'tvm' -> shelfState
- Type/file alignment: MinimalFaceFeatures->AnimatedFace,
  AudioSpectrum(View)->MusicVisualizer(Model), LiveActivityStackView->
  LiveActivityStack, PermissionRequestView->PermissionsRequestView,
  CameraPreview(Layer)View->WebcamPreview(Layer)
- Removed spaces from 'Live activities/' and 'YouTube Music Controller/'
  directories (shell-tooling hazard), pbxproj updated

Conventions & safety:
- 21 classes finalized; app-target 'public' noise reduced to internal
- ~20 scattered bundle-ID literals centralized in MediaAppBundleID
- Deleted unused utils/Logger.swift (never compiled); fixed stale header
  filename in matters.swift

Latent bugs fixed (3.5):
- Closed-notch OSD slider now actually routes drags to hardware: the
  sendEventBack closure is wired into DraggableProgressBar.onChange
  (was silently display-only)
- Calendar reloads coalesce: EventKit change bursts no longer trigger
  back-to-back full reloads (2s debounce)
- Contact avatar + lyrics caches use NSCache (evictable) instead of
  session-unbounded dictionaries

Dedup:
- Color+AccentColor: single decode helper replaces 4 near-identical blocks
- AppleScript controllers share executeCommand + player-info observation
  via AppleScriptControllerSupport

Logging:
- os.Logger backbone (helpers/Log.swift) with 11 feature categories;
  all ~80 production print() calls converted with level heuristics
  (debug vs error)

Verified: Debug build succeeds for app + XPC helper
Community health (4.1):
- Add Contributor Covenant 2.1 CODE_OF_CONDUCT.md with project contacts
- Add CODEOWNERS (default owner + area-specific rules)

Tests (4.2) — the 'Build & Test' badge is now truthful:
- New boringNotchTests XCTest target wired into the project + shared
  scheme TestAction (5 passing tests: NotchUIEventBus contract,
  MediaAppBundleID invariants, PlaybackState Equatable semantics — one of
  which caught a doc/code mismatch in the audit's claims)
- cicd.yml gains a Test step (hosted app tests, debug config)

Lint (4.3):
- .swiftlint.yml: force_unwrapping/force_cast/force_try as errors with the
  handful of audited legacy sites excluded per-rule; opt-in rules matching
  the house style; length limits from the audit metrics
- CONTRIBUTING style guide uncommented and made concrete (conventions
  established during the remediation, license-header policy)

Repo honesty (4.4):
- dependabot: drop no-op swift section with documented reason; route PRs
  to dev
- README: drop the stale 'no Apple Developer account' claim; describe
  actual notarization state
- SECURITY.md: supported-versions table, private-API risk notes, XPC
  helper privilege model (the repo's highest-trust component), vendor
  binary notes

Legal/copyright (4.5):
- NSHumanReadableCopyright populated in all build configs
- SPDX GPL-3.0-only headers on every file created during this branch;
  convention for existing files documented in CONTRIBUTING

Verified: xcodebuild Debug build + xcodebuild test (5/5 passing)
The app target has CODE_SIGN_ALLOW_ENTITLEMENTS_MODIFICATION=YES, which
requires a cert from a real team — an empty team with 'Apple Development'
identity fails with 'requires a development team'. Automatic style +
restored team ID picks the available local cert automatically; CI still
overrides DEVELOPMENT_TEAM at build time. The XPC helper stays ad-hoc
(team-less), which continues to sign 'to Run Locally' fine.

Forks: change DEVELOPMENT_TEAM to your own team (or clear it and write a
README note) when building locally.
theboringhumane and others added 3 commits August 30, 2026 12:52
- SECURITY.md: drop the Supported Versions table, keep the prose
- Revert .gitignore cleanup that over-reached (restore origin/dev version
  with *.xcscheme) and untrack the shared boringNotch.xcscheme
- dependabot.yml: restore the swift package-ecosystem section (it works
  with the xcodeproj-embedded manifest here; produced PR #1434)

Co-authored-by: TheBoringMajdoor <themajdoor@theboring.name>
- New .github/workflows/swiftlint.yml: lints PRs to dev and stack/**
  on a macOS runner (brew install guarded by which, no --strict)
- .swiftlint.yml: drop narrative comment lines, keep all functional
  config; the CI follow-up note is now obsolete since CI exists

Co-authored-by: TheBoringMajdoor <themajdoor@theboring.name>
…trim

- NotchHomeView music slider: 0.5s -> 0.2s TimelineView ticks (0.5s
  visibly steps; 0.2s is the cap the slider tolerates)
- WebcamPreview -> WebcamView to match the filename and the
  views-get-View-suffix convention (WebcamPreviewLayer unchanged)
- SystemEventIndicatorModifier: drop narrating doc comment on
  sendEventBack, keep the property

Co-authored-by: TheBoringMajdoor <themajdoor@theboring.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants