Repository navigation
Land review follow-ups (#3) on main - #5
Merged
Merged
Conversation
AppState handled login, polling, settings and repo selection. The persisted preferences and the repo selection helpers now live in a small SettingsStore, and the snapshot used by the Reset button is view state in SettingsView. Both classes use @observable instead of ObservableObject; views read AppState from the environment and bind to the store with @bindable. UpdaterService stays an ObservableObject because it bridges Sparkle's KVO property with Combine. No behavior change: same UserDefaults keys, same save points. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Edits in the settings panel were applied in memory right away but only written to UserDefaults when Done was pressed, so closing the menu without Done left the app running with settings that a relaunch would lose. Each setting now saves itself when it changes, like the polling interval already did. Done just closes the panel and refreshes. Since a non-empty selection is now always saved, checking whether the selectedRepositories key exists is redundant; an empty selection alone decides whether to fall back to all repos. The Reset button still restores the selection from when the panel was opened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Completion notifications only fired for runs previously seen as in_progress or queued. RunStatus also treats waiting, pending and requested as queued, and runs in those states completed silently. The rule now uses RunStatus.isActive, and lives in [WorkflowRun].newlyCompleted(since:) so it can be tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A workflow runs response that failed to decode made fetchRunsForRepo return [] as if the repo had no runs. It now throws decodingError like the other requests. fetchRecentRuns still returns partial results when only some repos fail, but now reports which ones did, and the menu shows "Couldn't load runs for <repo>" (or "for N repositories", with the names in a tooltip) above the list. fetchAvailableRepos ignored errors entirely, leaving an empty or stale list with no explanation. It still keeps the previous list on failure, but settings now shows the error under the repository header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
appState.start() runs from the MenuBarExtra label's onAppear, which can fire more than once. Each extra call validated the token again and started another monitoring pass, restarting polling. start() now ignores every call after the first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Swift 6 turns on complete concurrency checking. The only thing it flagged was AppDelegate mutating NSPanel.collectionBehavior, which is main actor-isolated, from the Sendable block-based observer. AppDelegate is now explicitly @mainactor, and the window observer is a selector-based observer calling a main actor method (windows become key on the main thread). The UNUserNotificationCenterDelegate methods are nonisolated, as they effectively were before: UserNotifications doesn't promise to call them on the main thread, and an isolated conformance would trap at runtime if it didn't. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Swift Testing coverage for the pure logic: - RunStatus(from:) for every status and conclusion, and isActive - [WorkflowRun].filtered(by:patterns:), limited(toRunsPerRepo:), overallStatus and the newlyCompleted(since:) notification rule - RelativeTime.format - Decoding the API and OAuth models with the app's decoder - SettingsStore loading, saving on change, and the sign-out reset The bundle has no test host. It compiles the model files it tests itself, so running the tests never launches the app. A launched, differently signed app could read the Keychain item, get errSecAuthFailed and delete the user's token. The JSON decoder setup moves to JSONDecoder.gitHubAPI() so the tests use the app's real configuration. The test target is in the shared scheme, CI runs it after the Release build, and there's a `make test` target. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Settings now save as they change, so a poll during editing used half-finished values. With an empty selection (after pressing None) it would replace the user's in-progress picks with the repos that have recent runs. Polls now use a snapshot taken when Settings opens, and Done applies the new settings. The Reset button uses the same snapshot instead of one taken in onAppear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The notification history only kept the visible runs, and was rebuilt from scratch on each fetch. A run hidden for one fetch (by the filter or the per-repo limit) or in a repo that failed to load for one fetch lost its "was running" status, so it never notified when it finished. RunHistory now records every fetched run and keeps the last status for repos that failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A refresh asked for while a fetch was in flight (like the one Done triggers) was dropped, so new settings could wait a full polling interval. It now runs once the current fetch finishes, and results fetched with settings that have since changed are discarded. Requests that finish after sign-out no longer write runs, the repo selection, errors or the username, or start polling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fetch threw the first per-repo error whenever no runs came back, so one failing repo next to repos with no runs (or Actions disabled) replaced the menu with a full error instead of the partial-failure line. Now it only fails outright when every requested repo failed, or on an invalid token or rate limit. The rule moves next to GitHubAPIError so it can be unit tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Changing the polling interval cancels the poll task mid-fetch. Its unfinished repos came back as network errors, so the menu could show some repos as failed, or "Network error: cancelled", and the new poll's first fetch was skipped because the old one was still in flight. A cancelled fetch now throws CancellationError and is ignored, and the new poll waits for the old one to end before fetching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A background installation check already in flight at sign-out could still call installationFound() and start polling, including for the next account when that account has no installation yet. The check now stops if the task was cancelled or the session changed, and waiting for an installation stops any poll that's running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening Settings and pressing refresh started two loads, and whichever finished last won. An older failure could show the red error under a list that had loaded, or an older list could replace a newer one, and the spinner stopped when the first one finished. A new load now cancels the previous one, and only the current load updates the list, the error and the spinner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves the AppDelegate conflict with "Only set the panel collection behavior when it's missing": keeps the selector-based observer that Swift 6 needs and adds the check that skips panels which already have the behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups: settings persistence, notifications, errors, tests, Swift 6
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
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.
PR #3 was stacked on PR #2's branch (
t3/5bd0c99d) and was merged into that branch after #2 had already landed, so its changes never reachedmain. This PR brings them over.Contents are exactly what was reviewed in #3: settings persistence via
SettingsStoreand Observation, notification fixes, error surfacing, theActionBarTeststarget, and the Swift 6 language mode. The branch merges intomainwithout conflicts.🤖 Generated with Claude Code