Refactor: Swift code quality improvements by superhighfives · Pull Request #184 · superhighfives/pika

superhighfives · 2026-03-09T20:32:10Z

Summary

Type-safe notifications: Added Notification.Name typed constants extension, replacing raw string literals throughout the codebase. Added convenience notification properties to Eyedropper.Types enum.
Swift Concurrency: Replaced all Combine timer subscriptions (Timer.publish + AnyCancellable) with Task/async-await in ContentView, SwapButtonStyle, EyedropperButton, KeyboardShortcutItem, and Toast.
Deduplication: Extracted shared Color.pikaControlBackground(for:) extension (eliminating duplicate getBackgroundColor() methods), and a shared @ViewBuilder appearanceSideOverlay function (eliminating duplicate overlay code).
Data-driven views: Replaced 220 lines of hardcoded KeyboardShortcutItem blocks in KeyboardShortcutGrid with a ShortcutEntry struct + ForEach.
PreferencesView decomposition: Split the monolithic 373-line view body into 6 private sub-view structs (GeneralAndSelectionSection, AppModeSection, AppearanceSection, CopySettingsSection, ColorFormatSection, GlobalShortcutSection).
Type safety: Replaced Any + String type erasure in ComplianceToggleGroup with an enum ComplianceData { case wcag; case apca }.
@State misuse: KeyboardShortcutItem configuration properties converted from @State to let.
Cula.swift split: Moved HSB/HSL logic to NSColor+HSL.swift and LAB/OKLCH/OpenGL logic to NSColor+Lab.swift, with a shared linearizeSRGB helper eliminating duplicated inline functions.
Dead code: Removed unused ColorCompliance protocol and its extensions.

Test plan

Build succeeds for both Pika (Sparkle) and Pika (Mac App Store) targets
Eyedropper picking (foreground and background) works correctly
All keyboard shortcuts in the shortcuts overlay trigger correctly
Copy actions (foreground, background, text, JSON) work
Swap button hover animation still works (250ms delay)
Color overlay timer in preferences shows/hides correctly
Toast dismiss animation still fires after 1 second
Preferences window shows all toggles including Float window option
WCAG and APCA compliance previews render correctly in preferences

🤖 Generated with Claude Code

@ViewBuilder

- Type-safe Notification.Name constants replacing raw string literals - Add typed notification helpers to Eyedropper.Types enum - Replace Combine timer subscriptions with Swift Concurrency (Task/async-await) in ContentView, SwapButtonStyle, EyedropperButton, KeyboardShortcutItem, Toast - Fix @State misuse: KeyboardShortcutItem properties converted to let constants - Deduplicate getBackgroundColor() via Color.pikaControlBackground extension - Deduplicate AppearanceButtonStyle overlay via shared @ViewBuilder function - Data-drive KeyboardShortcutGrid with ShortcutEntry structs (220 → ~60 lines) - Decompose PreferencesView into 6 private sub-view structs - Replace Any type erasure in ComplianceToggleGroup with enum ComplianceData - Consolidate WCAG/APCA conditional logic in Footer with computed properties - Split Cula.swift into NSColor+HSL.swift and NSColor+Lab.swift extensions - Remove dead ColorCompliance protocol - Register new extension files in Xcode project (both targets) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR is a comprehensive code quality refactoring of the Pika macOS color picker app. It focuses on modernizing Swift patterns, deduplicating shared logic, and improving type safety across the codebase without changing any user-facing behavior.

Changes:

Replaces Combine-based timer subscriptions (Timer.publish/AnyCancellable) with Swift Concurrency (Task/async-await) throughout views and styles
Introduces typed Notification.Name constants and a ComplianceData enum to eliminate raw string comparisons and Any type erasure
Refactors large views (PreferencesView, KeyboardShortcutGrid) into smaller, focused sub-views and data-driven components, and splits Cula.swift into focused extension files

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`Pika/Views/Toast.swift`	Replaces `DispatchQueue.main.asyncAfter` with `.task(id:)` for dismiss animation
`Pika/Views/PreferencesView.swift`	Splits 373-line view body into 6 private sub-view structs
`Pika/Views/KeyboardShortcutItem.swift`	Converts config `@State` to `let`; adopts typed notification name
`Pika/Views/KeyboardShortcutGrid.swift`	Replaces 200 lines of hardcoded items with `ShortcutEntry` struct + `ForEach`
`Pika/Views/EyedropperItem.swift`	Adopts typed notification names via `Eyedropper.Types` computed properties
`Pika/Views/EyedropperButton.swift`	Replaces Combine hover timer with async `Task`
`Pika/Views/ContentView.swift`	Replaces Combine swap-hide timer with async `Task`
`Pika/Views/ComplianceToggleGroup.swift`	Replaces `Any`+`String` type erasure with typed `ComplianceData` enum
`Pika/Views/ComplianceButtons.swift`	Updated to use `ComplianceData` enum
`Pika/Views/Footer.swift`	Extracts compliance/contrast logic into computed properties
`Pika/Styles/SwapButtonStyle.swift`	Replaces Combine timer with async `Task`; uses shared `pikaControlBackground`
`Pika/Styles/CircleButtonStyle.swift`	Removes dead `@State isHovered`; uses shared `pikaControlBackground`
`Pika/Styles/AppearanceButtonStyle.swift`	Extracts shared `appearanceSideOverlay` free function; lifts base colors to file scope
`Pika/Extensions/Color+PikaControl.swift`	New shared `Color.pikaControlBackground(for:)` extension
`Pika/Extensions/NSColor+HSL.swift`	New file with HSB/HSL logic moved from `Cula.swift`
`Pika/Extensions/NSColor+Lab.swift`	New file with OpenGL/LAB/OKLCH logic + shared `linearizeSRGB` helper
`Pika/Extensions/Cula.swift`	Reduced to core utilities after moving color space logic to new extension files
`Pika/Constants/Constants.swift`	Adds `Notification.Name` extension with typed constants wrapping raw string constants
`Pika/Utilities/Eyedroppers.swift`	Adds `pickNotification`, `copyNotification`, `systemPickerNotification` to `Types` enum
`Pika/AppDelegate.swift`	Updated to use typed `Notification.Name` constants
`Pika.xcodeproj/project.pbxproj`	Registers new extension files for both build targets

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-09T20:39:04Z

+                    hoverTask = Task {
+                        try? await Task.sleep(for: .milliseconds(100))
+                        isHovered = true
                    }


In SwapButtonStyle, when hover enters, a new Task is created directly without first cancelling any existing hoverTask. If two consecutive hover-enter events fire without an intervening hover-exit (e.g., during rapid view re-renders), the first task's reference is overwritten without being cancelled. While the practical impact is small (100ms max lifetime), this is inconsistent with the pattern used in EyedropperButton and ContentView, where hover-enter explicitly cancels the pending task before creating a new one. Adding hoverTask?.cancel() before the new task assignment would make this robust.

@copilot open a new pull request to apply changes based on this feedback

Split remaining Cula.swift content into four extension files: - NSColor+Init.swift: convenience initialisers (r/g/b and hex string) - NSColor+Luminance.swift: luminance, contrastRatio, toContrastRatioString - NSColor+Hex.swift: roundToHex, toHex, toHexString - NSColor+RGB.swift: toRGBAComponents, toRGBString, toRGB8BitArray, toFormat, getUIColor Deletes Cula.swift entirely. Register all new files in both Xcode targets. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tyle Consistent with the pattern used in EyedropperButton and ContentView. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- PikaTests XCTest target hosted against Pika.app (both targets in scheme) - 9 test files covering NSColor extensions, WCAG/APCA compliance, and Notification.Name constants (75+ test cases total): - NSColorInitTests: init(r:g:b:a:) normalisation, init(hex:) parsing - NSColorLuminanceTests: clip, luminance, contrastRatio - NSColorHexTests: roundToHex, toHex, toHexString round-trip - NSColorRGBTests: toRGBAComponents, toRGBString, toRGB8BitArray, toFormat, getUIColor - NSColorHSLTests: toHSBComponents/String, toHSLComponents/String - NSColorLabTests: toOpenGLString, toLabComponents/String, toOklchComponents/String - WCAGComplianceTests: 3:1/4.5:1/7:1 boundary cases, symmetry, cumulativity - APCAComplianceTests: all level strings, value formatting, symmetry - NotificationNamesTests: all 19 typed Notification.Name constants vs raw strings - .github/workflows/tests.yml: runs on every PR and push to main (macOS-15, Xcode 16.3) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Add default = .css to toLabString(style:) and toOklchString(style:) (these were missing default values unlike all other format functions) - Add missing import SwiftUI to NSColorRGBTests.swift Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Xcode 15+ treats any script phase stderr output as a test failure, even when the script exits 0. The LaunchAtLogin copy-helper script outputs a keychain error in CI (no signing cert), which caused xcodebuild to abort before running any tests. Split into two steps: - build-for-testing: compiles everything, tolerates script stderr - test-without-building: runs tests via .xctestrun, no script phases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Remove xcpretty from test-without-building so failures are visible in CI logs. Broaden .xctestrun glob pattern and add diagnostic output when the file is not found. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rgb(180,180,180) on black gives ~10:1 (passes ratio70), not ~5.7:1. Use rgb(120,120,120) on black which gives ~4.76:1 as intended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot · 2026-03-10T00:13:05Z

@superhighfives I've opened a new pull request, #185, to work on those changes. Once the pull request is ready, I'll request review from you.

AppDelegate (488→225 lines, 4 swiftlint disables removed): - Extract URLSchemeHandler — handles pika:// URL scheme dispatch, replacing the cyclomatic_complexity/function_body_length suppressions - Extract WindowCoordinator — owns all window creation and visibility - Extract StatusBarController — owns status bar setup, visibility, and click handling (now observes Defaults internally) - AppDelegate reduced to thin lifecycle + notification dispatch layer PreferencesView (2 swiftlint disables removed): - Replace 3-element tuple return from getColorSpaces() with a private ColorSpaceConfig struct, removing large_tuple and opening_brace disables Folder reorganisation: - Utilities/ → Services/ (rename + 3 new files added) - Styles/ → ButtonStyles/ (rename) - Views/ Xcode groups reorganised into logical subgroups: Eyedropper, Compliance, Preferences, About, Navigation, Shared - Services/ Xcode groups: Models, Export, Windows, ColorNames, StatusBar, URLHandling, Modifiers All 6 previously-suppressed swiftlint rules now pass without disables. Remaining disables (identifier_name in math code, line_length for long strings) are legitimate and unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

…ivity - SwapButtonStyle: remove internal onHover (child hover was breaking parent hover state, causing flicker); alt text now shows when button is visible - EyedropperButton, ContentView: add nil-guard on hide task creation to prevent rapid task churn when hover events fire at boundaries (mirrors the original Timer.publish guard behaviour) - WCAGComplianceTests: rename single-char vars (a→colorA etc.) and remove trailing comma to satisfy SwiftLint identifier_name/trailing_comma rules - NSColorHexTests, NSColorRGBTests, NSColorHSLTests: pin Defaults[.colorSpace] to sRGB in setUp so string-output tests pass regardless of display profile; fix two roundtrip tests to convert calibrated-RGB input to sRGB explicitly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…omma conflict - NSColor+HSL: normalize HSB hue with truncatingRemainder so pure red returns 0° (h=0.0) instead of 360° (h=1.0) — fixes user-visible bug where the app displayed "hsb(360, 100%, 100%)" for red - APCAComplianceTests: rename fg→foreground, bg→background, a→colorA, b→colorB to satisfy identifier_name; remove trailing comma from pairs array - .swiftlint.yml: disable trailing_comma — Xcode's formatter adds trailing commas automatically (Swift convention), creating an unresolvable conflict; disabling the rule aligns linting with the project's actual formatting practice - Import order + blank-line fixes applied by Xcode formatter accepted as-is Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add onHoverChange callback to SwapButtonStyle so parent views know when a child button is hovered. SwiftUI fires inner .onHover before outer, so by the time the parent's false-handler runs, childHovered/swapButtonHovered is already true → the parent skips starting the 250ms hide task. - SwapButtonStyle: restore @State isHovered + 100ms task for alt text; add onHoverChange: ((Bool) -> Void)? parameter; guard hoverTask == nil (matches original Timer.publish behaviour) - EyedropperButton: add childHovered flag; wire onHoverChange on both overlay buttons; skip hide task when childHovered is true - ContentView: add swapButtonHovered flag; wire onHoverChange on swap button; skip hide task when swapButtonHovered is true Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… cancellation guard - Animate isHovered transitions with easeInOut(0.15s) for smooth icon→label expand - Add hoverCooldown (150ms) after cursor exits to block animation-induced re-entry - Fix Task cancellation: guard !Task.isCancelled after try? sleep so cancelled tasks don't execute their side-effects (was causing labels to get stuck open) - Add onHoverChange callback to propagate child hover state to parent - Use childHovered flag in EyedropperButton to suppress premature hide task - Use Timer.publish in ContentView for stable swap button visibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 9, 2026 20:32

Copilot started reviewing on behalf of superhighfives March 9, 2026 20:32 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

superhighfives and others added 8 commits March 9, 2026 13:43

Fix: cancel existing hoverTask before creating new one in SwapButtonS…

4da9e2c

…tyle Consistent with the pattern used in EyedropperButton and ContentView. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix test compilation errors

b388de8

- Add default = .css to toLabString(style:) and toOklchString(style:) (these were missing default values unlike all other format functions) - Add missing import SwiftUI to NSColorRGBTests.swift Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix CI: broaden xctestrun search and show raw test output

619f311

Remove xcpretty from test-without-building so failures are visible in CI logs. Broaden .xctestrun glob pattern and add diagnostic output when the file is not found. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add workflow_dispatch trigger to allow manual test runs

b5cbb11

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix WCAGComplianceTests: use gray with contrast in 4.5-7:1 range

83d46de

rgb(180,180,180) on black gives ~10:1 (passes ratio70), not ~5.7:1. Use rgb(120,120,120) on black which gives ~4.76:1 as intended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI mentioned this pull request Mar 10, 2026

Cancel existing hoverTask before creating new one in SwapButtonStyle #185

Merged

superhighfives and others added 9 commits March 9, 2026 17:45

Initial plan (#185)

ff2e19b

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Fix menu width

a3b1a8c

Fixed spacing and menu

5741e41

Fixed it

0573e9a

superhighfives merged commit 55c0fce into main Mar 10, 2026
1 check passed

superhighfives deleted the feature/code-cleanup branch March 10, 2026 18:27

Navigation Menu

Refactor: Swift code quality improvements - #184

Refactor: Swift code quality improvements#184
superhighfives merged 18 commits into
mainfrom
feature/code-cleanup

superhighfives commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

superhighfives Mar 10, 2026

Uh oh!

Copilot AI commented Mar 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

superhighfives commented Mar 9, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

superhighfives Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Mar 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants