refactor: remove dead code and reduce redundancy project-wide#4
Merged
Conversation
Dead tokens with zero references anywhere in Sources/ or Tests/: Spacing.xxl,
the entire Radius enum, Size.{rowIcon,overlayButton,updateHeaderIcon,progressBar},
Window.{aboutWidth,updateWidth,updateHeight}, Palette.danger, Font.chip,
Motion.{toggle,resultDwell}. Size.overlayButton duplicated OverlayGeometry.buttonSize
(the value the live overlay lays out against), which remains the single source of truth.
Build: zero warnings.
Confirmed-dead, zero production consumers (compiler/tests verified): - EventTap.isRunning (no consumer) - AppKitStrings.string variadic CVarArg overload (only single-arg form used) - WindowInfo.title (parsed from kCGWindowName but never read) + 7 test call sites - WindowCapabilities.hasAny (test-only) + its test - UpdateChannel: drop Identifiable/CaseIterable/id (no Picker/ForEach consumer; UpdateController uses only the kept static funcs) + its test Build+tests: zero warnings, all green.
- OverlaySettingsTests.lenientDecode now omits showMinimize/showZoom so the init(from:) `decodeIfPresent ?? true` fallback is actually exercised (the old blob set all three keys, so it was just a second round-trip decode). - WindowInfoTests: extract the duplicated main-display CGRect into a `mainDisplay` property; the empty-displays case is untouched. make test: all green, zero warnings.
…fix Log doc - Add View.settingsFooter() and use it in General/Updates/Shortcuts panes, replacing the 6x inline .font(DS.Font.sectionFooter).foregroundStyle(.secondary) (render byte-identical). - showSettingsForScreenshot() reuses View.localized(with:) instead of hand-rolling the same environment(\.locale)/.id trio. - restartEngine() is now private (only called within AppState). - Log.swift doc example now uses /usr/bin/log ... --info --style compact per the CLAUDE.md diagnostics rule (bare `log` is a zsh builtin). make build: zero warnings.
Behavior-preserving dedup in the Mission Control engine (no value, order, or control-flow change — only grouping repeated statement runs into private helpers): - clearOverlayContent(): the orderOut + geometry=nil + currentActions=[] triple, applied at the two sites where it is exactly consecutive (repositionOverlay guard #1, hideOverlay). Left inline where not a clean match: repositionOverlay guard #2 (uses the unwrapped local window), endSession (hovered=nil interleaves). - resetSettleState(): the layoutSettled/stableTicks/sinkWatchTicks reset, at beginSession/resyncSession/endSession; the distinct per-site comments are preserved on the call lines. make build + make test: zero warnings, all green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (5)
Summary by CodeRabbit
WalkthroughSeveral public API members are removed: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
Comment |
BlackHole1
added a commit
that referenced
this pull request
Jun 30, 2026
…IME) (#5) ## Problem The Nightly (Beta) / Release publish job fails at **Build appcasts** with exit 1, right after: ``` Wrote 1 new update, updated 0 existing updates, and removed 0 old updates in appcast-arm64.xml ``` `test -f "build/dist-$ARCH/appcast.xml"` then fails. ## Root cause `generate_appcast` can't run the app, so it derives the output appcast **filename from the app's `SUFeedURL` basename**. The per-arch split (#3) set `Info.plist` `SUFeedURL` to `…/appcast-arm64.xml`, so the tool writes `appcast-arm64.xml` in **every** `build/dist-$ARCH` dir — but the CI seeds, tests, and publishes the **local** file `build/dist-$ARCH/appcast.xml`. Mismatch → exit 1. (The log line `…in appcast-arm64.xml` is the derived name, not the file actually written.) ## Fix — mirror LockIME (no `-o`) - Revert `Sources/CloseUp/Info.plist` `SUFeedURL` → `…/appcast.xml`. It is a **runtime no-op** (`UpdaterDelegate.feedURLString` pins an explicit per-arch URL for both arches); its only jobs are the `generate_appcast` naming basis and the 0.1.0 default. With basename `appcast.xml`, the tool writes the local `appcast.xml` the CI already expects — **no `-o` needed**. - The local working file in each `build/dist-$ARCH` is always `appcast.xml` (the arch is already in the directory name). The per-arch feed name is applied **only at the gh-pages boundary**: seed `appcast-$ARCH.xml` → local `appcast.xml`; publish local `appcast.xml` → `appcast-$ARCH.xml`. - **CloseUp keeps symmetric feeds** (`appcast-arm64.xml` / `appcast-x86_64.xml`); `appcast.xml` stays a frozen 0.1.0 orphan the pipeline never writes. Unlike LockIME (legacy arm64-only, so its arm64 feed *is* `appcast.xml`), CloseUp's universal 0.1.0 also runs on Intel — arm64 entries in `appcast.xml` would push an arm64 build to an Intel 0.1.0. - Also pass `inputs.tag`/`channel` + `github.repository` to the step via `env` instead of interpolating `${{ }}` into the bash body (shell-injection hardening, per CodeRabbit). ## Verification - **Empirical** (Sparkle's prebuilt `generate_appcast`, basename `appcast.xml`, no `-o`): writes local `appcast.xml`, adds the new entry, preserves a seeded old entry's signature, creates no stray `appcast-*.xml`. - **Adversarial review** across 4 lenses (CI-correctness, LockIME-parity, 0.1.0-orphan/cross-arch, completeness): 0 confirmed defects. Docs updated (`AGENTS.md`, `docs/RUNBOOK.md`); the `-o` approach is removed. Rebased onto current `main` (includes #4). Note: `ci.yml` (this PR's checks) builds/tests but does **not** call `build-publish.yml`; only the scheduled nightly / manual release dispatch exercise the publish path.
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.
A whole-project audit (dead code, redundancy, and missing abstractions) turned up only low-risk cleanups — the codebase was already healthy and no load-bearing Mission Control logic was changed. This branch applies the verified findings: a net −60 lines across 17 files, with every commit building at zero warnings and the full test suite green.
Dead code removed (zero references anywhere in
Sources/orTests/, compiler/test-verified):DSdesign tokens — the wholeRadiusenum,Spacing.xxl, severalSize/Windowtokens,Palette.danger,Font.chip, andMotion.toggle/resultDwell.Size.overlayButtonalso duplicatedOverlayGeometry.buttonSize, which stays the single source of truth.EventTap.isRunning, the variadicAppKitStrings.stringoverload,WindowInfo.title(parsed fromkCGWindowNamebut never read),WindowCapabilities.hasAny(test-only), andUpdateChannel's unusedIdentifiable/CaseIterable/id— each with its tests updated in lockstep.Redundancy and abstraction:
View.settingsFooter()to replace six inline copies of the section-footer styling, and routed the debug screenshot path through the existingView.localized(with:)helper instead of re-rolling the locale-injection trio.MissionControlEngine, grouped two repeated statement runs intoclearOverlayContent()andresetSettleState(). These are behavior-preserving — no value, order, or control-flow change — and were applied only where the statements were exactly consecutive; the distinct per-site comments at each settle reset are preserved.Tests:
OverlaySettingsTests.lenientDecodenow omits keys so it actually exercises thedecodeIfPresent ?? defaultfallback it was named for; it previously set every key and was just a second round-trip decode.restartEngine()toprivate, and fixed theLog.swiftdiagnostics doc-comment to follow the project's/usr/bin/log … --inforule.MissionControlEngineis hot-path overlay code. The changes there are provably behavior-preserving and pass build + tests, but per the project's on-screen-verification rule a final manual Mission Control pass (hover → click → re-tile) is worth doing before merge.