Fix/popover scrollview jobs display (design-branch-1)#369
Closed
eonist wants to merge 74 commits into
Closed
Conversation
🔴 Bugs: - #1 actionStatusLabel → uppercase per spec (IN PROGRESS / SUCCESS / FAILED / CANCELED) - #2 Child ↳ row status text → uppercase (IN PROGRESS / QUEUED) - #3 [×] button → NSApplication.shared.hide(nil) instead of terminate(nil) - #4 Child PieProgressView progress → real step completion fraction 🟡 Polish / Spec: - #5 Fix ScrollView/LazyVStack/ForEach indentation alignment - #6 Simplify Load more label to static "Load 10 more actions…" - #7 Remove Text("Actions") section label - #8 Runner rows → Button wrapper + chevron.right - #9 Remove Text("Runners") section label - #11 Add .onDisappear { systemStats.stop() } 📝 Docs: - #10 Fix RunnerStoreObservable comment: "capped at 5" → "capped at 10"
- #5 PieProgressView partial state: replace .stroke ring arc with Path filled wedge so partial progress renders as a true pie slice (◔ ◑ ◕) - #6 RunnerStoreObservable: add @mainactor for compile-time thread safety - #8 Auth dot: route to onSelectSettings instead of signInWithGitHub() - #9 Inline ↳ job rows: make passive (remove Button/onSelectJob/chevron)
- nesting: extract stepProgress(for:) helper to avoid function_level > 3 - function_body_length: split body into actionsSection + runnersSection @ViewBuilder sub-views to stay under 50-line warning threshold - missing_docs: add /// to all new internal helpers
…llapse + RunnerStore runners (#296) - RunnerStoreState: actions cap 5→50, jobs cap 3→30 — enables pagination (#305) - PopoverMainView: remove LocalRunnerStore, drive runners from store.runners (#307) - PopoverMainView: add @State expandedGroups per action group, expand by default for inProgress (#304) - PopoverMainView: extract PopoverHeaderView, ActionRowView, InlineJobRowView, RunnersListView subview structs (#296) Ref #296 #304 #305 #307
…r_is_empty, multiple_closures_with_trailing_closure)
- hasInlineJobs: filter{}.isEmpty → contains{} (contains_over_filter_is_empty)
- ActionRowView init: use explicit argument labels for onToggleExpand + onSelect closures
- Button(action:) in PopoverHeaderView: use label: parameter explicitly instead of trailing closure
- Button(action:) load-more: use label: parameter explicitly
Ref #296
- RunnersListView: disable no-op runner button until #307 detail is wired (removes misleading tappable chevron) - PopoverMainView: reset visibleCount to 10 on store.actions.count change - RunnerStoreObservable: fix doc comment ('capped at 10/3' refers to visibleCount in view, not store cap)
Replace the static "BUSY" / "ONLINE" status label in RunnersListView with runner.displayStatus, which already formats as "active (CPU: x.x% MEM: x.x%)" or "idle (CPU: — MEM: —)". The Runner model and RunnerMetrics are already populated by RunnerStore.fetch() — this was the only missing wiring. Closes the one remaining gap flagged in the overall verdict comment.
- Gap 1 (#304): InlineJobRowView shows currentStepTitle + stepFraction instead of status label - Gap 2 (#304): hasInlineJobs + InlineJobsView filter to in_progress only (drop queued) - Gap 3 (#305): 'No more actions' label already present — confirmed correct - Gap 4 (#307): RunnersListView shows CPU% + MEM% from runner.metrics; falls back to em-dash - Bonus (#299): hide() confirmed correct for menu-bar app; tooltip updated to 'Close popover' Closes #323
…on_body_length) - Add /// docs to all undocumented private funcs in ActionRowView + InlineJobRowView - Add scoped swiftlint:disable file_length (file is a single cohesive view decomposition) - Add swiftlint:disable:next function_body_length on ActionRowView.body (SwiftUI body verbosity)
…etrics` (identifier_name)
- SystemStatsViewModel: init() is no-op; serial samplingQueue; explicit start()/stop() - PieProgressView: progress: Double? with nil indeterminate centre dot - RelativeTimeFormatter: standalone testable enum with injectable relativeTo: - ActionGroup.progressFraction + ActiveJob.progressFraction model extensions - ActionRowView + InlineJobRowView: use progressFraction extensions - InlineJobsView: @State cap=4 with load-more button (replaces hard prefix(5)) - PopoverMainView: .onChange observes full store.actions array (not just count) - PopoverHeaderView: Sign in caption next to orange auth dot - PopoverHeaderView: blockBar Unicode prefix on stat chips (spec #294/#296) Ref: #363 #364 #365 (closed as duplicates)
…InlineJobRowView - ActiveJob.progressFraction: Double? — model-layer extension matching ActionGroup.progressFraction pattern; nil = indeterminate (queued / no steps) - InlineJobRowView: drop local jobProgressFraction(for:) helper, use job.progressFraction directly (same as ActionRowView uses actionGroup.progressFraction) Ref: #366 minor observation
- RunnerStore.swift: add scoped file_length disable (411 lines, cohesive singleton) - GitHub.swift: rewrite DispatchWorkItem(block:) as trailing closure to satisfy multiple_closures_with_trailing_closure rule
- PopoverMainView.swift: rewrite all Button(action:label:) as Button(action:) { }
trailing-closure form to satisfy multiple_closures_with_trailing_closure
- RunnerStore.swift: remove superfluous file_length disable (rule fires on Logger.swift
at lint position 13, not RunnerStore; disable was in wrong file)
- GitHub.swift: add scoped file_length disable (pre-existing, file is 411 lines)
- PopoverMainView.swift: rewrite all Button(action:) { } back to Button(action:label:) explicit
form — SwiftLint multiple_closures_with_trailing_closure fires on Button because it has
two closure params (action + label); explicit label: arg suppresses the rule
- PopoverMainView.swift: remove superfluous function_body_length disable on ActionRowView.body
…Store.swift file_length is a whole-file rule — scoped disable/enable pairs don't suppress it. Only a top-of-file disable (no matching enable) works correctly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Toggles the popover open or closed. | ||
| @objc private func togglePopover() { | ||
| @MainActor @objc private func togglePopover() { | ||
| guard let popover else { return } |
There was a problem hiding this comment.
🟠 navigate() resize reverses the long-standing regression guard — left-jump risk
The top-of-file REGRESSION GUARD (and 30+ prior issues #52 #54 #57 #59) explicitly prohibited touching contentSize while the popover is shown. This PR introduces exactly that: navigate() now calls hc.view.setFrameSize and popover.contentSize = size inside a DispatchQueue.main.async block that fires while the popover IS open (isShown == true). The comment says '!isShown guard is intentionally absent', but the historical record shows this causes NSPopover to re-anchor (left-jump) on every rootView swap. The one-async-tick delay does not prevent NSPopover from re-anchoring — it just defers the trigger. Recommend verifying this has been tested against the left-jump regression on the target hardware before merging, and updating the REGRESSION GUARD comment to document the new contract and why it is now safe.
Suggested change
| guard let popover else { return } | |
| // If resize-on-navigate is intentional, document the tested outcome: | |
| // ⚠️ REGRESSION GUARD UPDATE (v0.35+): | |
| // navigate() now resizes after each swap. Tested on [macOS version / hardware]. | |
| // NSPopover re-anchor was not observed because [reason]. | |
| // The !isShown guard is absent intentionally — see PR #369. | |
| private func navigate(to view: AnyView) { | |
| hostingController?.rootView = view | |
| guard let hc = hostingController, let popover else { return } | |
| DispatchQueue.main.async { [weak self] in | |
| guard let self else { return } | |
| let fitting = hc.view.fittingSize | |
| let width = fitting.width > 0 ? fitting.width : Self.fixedWidth | |
| let height = min(fitting.height > 0 ? fitting.height : 300, Self.maxHeight) | |
| let size = NSSize(width: width, height: height) | |
| hc.view.setFrameSize(size) | |
| popover.contentSize = size | |
| } | |
| } |
AI Fix Prompt
Fix the following High (Bug) issue in `Sources/RunnerBar/AppDelegate.swift` at lines 255-266:
Problem: The top-of-file REGRESSION GUARD (and 30+ prior issues #52 #54 #57 #59) explicitly prohibited touching contentSize while the popover is shown. This PR introduces exactly that: navigate() now calls hc.view.setFrameSize and popover.contentSize = size inside a DispatchQueue.main.async block that fires while the popover IS open (isShown == true). The comment says '!isShown guard is intentionally absent', but the historical record shows this causes NSPopover to re-anchor (left-jump) on every rootView swap. The one-async-tick delay does not prevent NSPopover from re-anchoring — it just defers the trigger. Recommend verifying this has been tested against the left-jump regression on the target hardware before merging, and updating the REGRESSION GUARD comment to document the new contract and why it is now safe.
Suggested fix:
// If resize-on-navigate is intentional, document the tested outcome:
// ⚠️ REGRESSION GUARD UPDATE (v0.35+):
// navigate() now resizes after each swap. Tested on [macOS version / hardware].
// NSPopover re-anchor was not observed because [reason].
// The !isShown guard is absent intentionally — see PR #369.
private func navigate(to view: AnyView) {
hostingController?.rootView = view
guard let hc = hostingController, let popover else { return }
DispatchQueue.main.async { [weak self] in
guard let self else { return }
let fitting = hc.view.fittingSize
let width = fitting.width > 0 ? fitting.width : Self.fixedWidth
let height = min(fitting.height > 0 ? fitting.height : 300, Self.maxHeight)
let size = NSSize(width: width, height: height)
hc.view.setFrameSize(size)
popover.contentSize = size
}
}
| id: payload.id, | ||
| name: payload.name, | ||
| status: payload.status, | ||
| conclusion: payload.conclusion, |
There was a problem hiding this comment.
🟠 JobStep.startedAt / completedAt passed as String? but JobStep stores Date? — compile or runtime mismatch
In makeActiveJob(from:iso:isDimmed:), the step construction now passes step.startedAt and step.completedAt directly (without ISO parsing) as arguments to JobStep(…). If JobStep.startedAt and .completedAt are Date? (as expected from the Codable/Identifiable struct), this is a type error — String? cannot be passed where Date? is expected. The previous code correctly called .flatMap { iso.date(from: $0) } on both fields. The change comment says 'startedAt: step.startedAt, completedAt: step.completedAt' as if these are already Date?, which would only be valid if JobStep was changed to hold String? in this PR. Please verify the JobStep struct's field types in ActiveJob.swift and confirm the ISO parsing is not silently dropped.
Suggested change
| conclusion: payload.conclusion, | |
| // Restore ISO parsing for JobStep timestamps: | |
| let steps: [JobStep] = (payload.steps ?? []).enumerated().map { idx, step in | |
| JobStep( | |
| id: idx + 1, | |
| name: step.name, | |
| status: step.status, | |
| conclusion: step.conclusion, | |
| startedAt: step.startedAt.flatMap { iso.date(from: $0) }, | |
| completedAt: step.completedAt.flatMap { iso.date(from: $0) } | |
| ) | |
| } |
AI Fix Prompt
Fix the following High (Bug) issue in `Sources/RunnerBar/ActionGroup.swift` at lines 306-313:
Problem: In makeActiveJob(from:iso:isDimmed:), the step construction now passes step.startedAt and step.completedAt directly (without ISO parsing) as arguments to JobStep(…). If JobStep.startedAt and .completedAt are Date? (as expected from the Codable/Identifiable struct), this is a type error — String? cannot be passed where Date? is expected. The previous code correctly called .flatMap { iso.date(from: $0) } on both fields. The change comment says 'startedAt: step.startedAt, completedAt: step.completedAt' as if these are already Date?, which would only be valid if JobStep was changed to hold String? in this PR. Please verify the JobStep struct's field types in ActiveJob.swift and confirm the ISO parsing is not silently dropped.
Suggested fix:
// Restore ISO parsing for JobStep timestamps:
let steps: [JobStep] = (payload.steps ?? []).enumerated().map { idx, step in
JobStep(
id: idx + 1,
name: step.name,
status: step.status,
conclusion: step.conclusion,
startedAt: step.startedAt.flatMap { iso.date(from: $0) },
completedAt: step.completedAt.flatMap { iso.date(from: $0) }
)
}
| return "#\(digits)" | ||
| } | ||
| return String(run.headSha.prefix(7)) | ||
| } |
There was a problem hiding this comment.
🟡 ActionRunsResponse / RunPayload / prLabel() promoted to internal — breaks encapsulation
The types ActionRunsResponse, RunPayload, HeadCommit, PRRef, and the function prLabel() were previously private to ActionGroup.swift. Moving them to a new file at module-internal scope means any file in the target can now instantiate and call them. These are raw API decoding types — they should not be part of the module's public surface. Mark them private or fileprivate, or add a // MARK: internal-only comment if the broader visibility is intentional (e.g. for testing).
Suggested change
| } | |
| // In ActionRunsResponse.swift, add access control: | |
| // swiftlint:disable missing_docs | |
| struct ActionRunsResponse: Codable { ... } // keep internal for now | |
| struct RunPayload: Codable { ... } // keep internal | |
| struct HeadCommit: Codable { ... } // keep internal | |
| struct PRRef: Codable { ... } // keep internal | |
| // prLabel is a free function — consider making it internal(set) or moving | |
| // it into an extension on RunPayload: | |
| extension RunPayload { | |
| var label: String { prLabel(from: self) } | |
| } |
AI Fix Prompt
Fix the following Medium (Architecture) issue in `Sources/RunnerBar/ActionRunsResponse.swift` at lines 6-82:
Problem: The types ActionRunsResponse, RunPayload, HeadCommit, PRRef, and the function prLabel() were previously private to ActionGroup.swift. Moving them to a new file at module-internal scope means any file in the target can now instantiate and call them. These are raw API decoding types — they should not be part of the module's public surface. Mark them private or fileprivate, or add a // MARK: internal-only comment if the broader visibility is intentional (e.g. for testing).
Suggested fix:
// In ActionRunsResponse.swift, add access control:
// swiftlint:disable missing_docs
struct ActionRunsResponse: Codable { ... } // keep internal for now
struct RunPayload: Codable { ... } // keep internal
struct HeadCommit: Codable { ... } // keep internal
struct PRRef: Codable { ... } // keep internal
// prLabel is a free function — consider making it internal(set) or moving
// it into an extension on RunPayload:
extension RunPayload {
var label: String { prLabel(from: self) }
}
| Circle().fill(Color.yellow).frame(width: 7, height: 7) | ||
| Text(runner.name) | ||
| .font(.system(size: 12)).foregroundColor(.primary) | ||
| .lineLimit(1).truncationMode(.tail) |
There was a problem hiding this comment.
🔵 RunnersListView silently drops the 'Active Jobs' drill-down — no onSelectJob callback
The original PopoverMainView rendered a tappable 'Active Jobs' section with onSelectJob drill-down to JobDetailView. RunnersListView renders busy runners but has no tap action and no chevron that leads anywhere functional (the chevron is rendered but no Button wraps the row). If this is an intentional product change (runners are display-only), the chevron icon should be removed to avoid misleading the user. If drill-down is planned, an onSelect callback should be wired.
Suggested change
| .lineLimit(1).truncationMode(.tail) | |
| // If runners are display-only, remove the misleading chevron: | |
| HStack(spacing: 6) { | |
| Circle().fill(Color.yellow).frame(width: 7, height: 7) | |
| Text(runner.name) | |
| .font(.system(size: 12)).foregroundColor(.primary) | |
| .lineLimit(1).truncationMode(.tail) | |
| Spacer() | |
| if let metrics = runner.metrics { | |
| Text(String(format: "CPU: %.1f%%", metrics.cpu)) | |
| .font(.caption.monospacedDigit()).foregroundColor(.secondary) | |
| Text(String(format: "MEM: %.1f%%", metrics.mem)) | |
| .font(.caption.monospacedDigit()).foregroundColor(.secondary) | |
| } else { | |
| Text("CPU: \u{2014} MEM: \u{2014}").font(.caption).foregroundColor(.secondary) | |
| } | |
| // Chevron removed — no drill-down implemented | |
| } | |
| .padding(.horizontal, 12).padding(.vertical, 3) |
AI Fix Prompt
Fix the following Low (Architecture) issue in `Sources/RunnerBar/PopoverMainView.swift` at lines 420-454:
Problem: The original PopoverMainView rendered a tappable 'Active Jobs' section with onSelectJob drill-down to JobDetailView. RunnersListView renders busy runners but has no tap action and no chevron that leads anywhere functional (the chevron is rendered but no Button wraps the row). If this is an intentional product change (runners are display-only), the chevron icon should be removed to avoid misleading the user. If drill-down is planned, an onSelect callback should be wired.
Suggested fix:
// If runners are display-only, remove the misleading chevron:
HStack(spacing: 6) {
Circle().fill(Color.yellow).frame(width: 7, height: 7)
Text(runner.name)
.font(.system(size: 12)).foregroundColor(.primary)
.lineLimit(1).truncationMode(.tail)
Spacer()
if let metrics = runner.metrics {
Text(String(format: "CPU: %.1f%%", metrics.cpu))
.font(.caption.monospacedDigit()).foregroundColor(.secondary)
Text(String(format: "MEM: %.1f%%", metrics.mem))
.font(.caption.monospacedDigit()).foregroundColor(.secondary)
} else {
Text("CPU: \u{2014} MEM: \u{2014}").font(.caption).foregroundColor(.secondary)
}
// Chevron removed — no drill-down implemented
}
.padding(.horizontal, 12).padding(.vertical, 3)
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+153
to
+162
| Button( | ||
| action: { NSApplication.shared.hide(nil) }, | ||
| label: { | ||
| Image(systemName: "xmark") | ||
| .font(.system(size: 11)) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| ) | ||
| .buttonStyle(.plain) | ||
| .help("Close popover") |
There was a problem hiding this comment.
Suggestion: The close control is labeled as closing only the popover, but it calls NSApplication.shared.hide(nil), which hides the entire app instead of explicitly closing the popover. This can produce incorrect behavior versus the UI contract and should be replaced with a true popover-close action. [logic error]
Severity Level: Major ⚠️
- ❌ Close button hides entire app instead of popover.
- ⚠️ Inconsistent with tooltip text "Close popover" behaviour.Steps of Reproduction ✅
1. Launch the RunnerBar app built from this PR so the status item is created and wired to
`togglePopover()` / `openPopover()` (see the popover show/hide section in
`Sources/RunnerBar/AppDelegate.swift:4-17` and `260-37`, where `popover.performClose(nil)`
is the canonical way to close the NSPopover).
2. Click the status bar icon to open the main popover. This calls `togglePopover()` →
`openPopover()` → `mainView()` (AppDelegate.swift:6-18), which returns a `PopoverMainView`
and embeds it in the `NSHostingController`. The header is rendered by `PopoverHeaderView`
(`Sources/RunnerBar/PopoverMainView.swift:101-165`), including the gear and "x" controls.
3. Hover over the "x" icon in the header and observe the tooltip text `"Close popover"`
coming from `.help("Close popover")` at `PopoverMainView.swift:161-162`, which
communicates that this control is intended to close only the popover.
4. Click the "x" button. Its `action` closure at `PopoverMainView.swift:153-160` executes
`NSApplication.shared.hide(nil)`, which hides the entire application rather than closing
the specific NSPopover instance (contrast with `AppDelegate.togglePopover()` at
`AppDelegate.swift:6-12`, which uses `popover.performClose(nil)`). The visible effect is
that the app (and any open windows) are hidden, requiring the user to reactivate the app
to see UI again, which violates the "Close popover" contract.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/RunnerBar/PopoverMainView.swift
**Line:** 153:162
**Comment:**
*Logic Error: The close control is labeled as closing only the popover, but it calls `NSApplication.shared.hide(nil)`, which hides the entire app instead of explicitly closing the popover. This can produce incorrect behavior versus the UI contract and should be replaced with a true popover-close action.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+334
to
341
| private struct InlineJobsView: View { | ||
| let jobs: [ActiveJob] | ||
|
|
||
| var body: some View { | ||
| ForEach(jobs) { job in | ||
| InlineJobRowView(job: job) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Inline job rows are rendered as plain views without any tap handler, and onSelectJob is never propagated into ActionsListView/InlineJobsView. This breaks the existing navigation contract from the main popover to job detail, so users can no longer open a job directly from the inline list. Thread the callback through and make each inline row actionable (e.g., wrap in a button). [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Inline job rows cannot open JobDetailView directly.
- ⚠️ Users must navigate via action detail view instead.Steps of Reproduction ✅
1. Launch the RunnerBar app built from this PR so that `AppDelegate.mainView()` is used as
the root popover content (see `Sources/RunnerBar/AppDelegate.swift:16-41`, where
`PopoverMainView` is created with an `onSelectJob: (ActiveJob) -> Void` callback wired to
navigate to `detailView(job:)` / `detailViewFromAction(job:group:)`).
2. Open the status bar popover; `togglePopover()` calls `openPopover()` which uses
`mainView()` to embed `PopoverMainView` as the popover content (see
`AppDelegate.swift:6-13` and `276-31`). `PopoverMainView.body` at
`Sources/RunnerBar/PopoverMainView.swift:37-61` renders
`ActionsListView(actions:store.actions, visibleCount:$visibleCount,
onSelectAction:onSelectAction)`.
3. Ensure there is at least one `ActionGroup` with `groupStatus == .inProgress` and
non-empty `jobs` in `RunnerStore.shared.actions`. `ActionsListView` renders these via
`ActionRowView` (`PopoverMainView.swift:195-200`), whose `showInlineJobs` computed
property (`lines 234-237`) becomes `true` and causes `InlineJobsView(jobs:
actionGroup.jobs.filter { $0.conclusion == nil })` to be rendered (`lines 281-287`),
displaying inline job rows in the main popover.
4. In the popover UI, click on one of the inline job rows. The inline list is produced by
`InlineJobsView` (`PopoverMainView.swift:334-341`), whose `body` is a plain `ForEach(jobs)
{ job in InlineJobRowView(job: job) }` without any `Button` or tap gesture.
`InlineJobRowView` itself (`lines 346-374`) is just an `HStack` with no `Button`,
`.onTapGesture`, or access to `onSelectJob`. As a result, tapping an inline job row does
nothing, and the `onSelectJob` callback threaded into `PopoverMainView` from
`AppDelegate.mainView()` (`AppDelegate.swift:19-30`) is never invoked, breaking direct
navigation from the main popover to `JobDetailView`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/RunnerBar/PopoverMainView.swift
**Line:** 334:341
**Comment:**
*Incomplete Implementation: Inline job rows are rendered as plain views without any tap handler, and `onSelectJob` is never propagated into `ActionsListView`/`InlineJobsView`. This breaks the existing navigation contract from the main popover to job detail, so users can no longer open a job directly from the inline list. Thread the callback through and make each inline row actionable (e.g., wrap in a button).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/RunnerBar/AddRunnerSheet.swift (1)
6-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing docs for internal declarations to unblock SwiftLint.
CI is currently failing on
missing_docs; this file’s internal declarations should be documented (AddRunnerSheet,ScopeType).Suggested fix
+/// Sheet for registering a self-hosted GitHub Actions runner. struct AddRunnerSheet: View { @@ + /// Target registration scope for the runner. enum ScopeType: String, CaseIterable, Identifiable {🤖 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 `@Sources/RunnerBar/AddRunnerSheet.swift` around lines 6 - 14, Add missing documentation comments for the internal declarations to satisfy SwiftLint missing_docs: add concise Swift doc comments (/// ...) above the AddRunnerSheet struct and the nested ScopeType enum explaining their purpose and usage, and document the properties `@Binding` var isPresented, let onComplete, and the enum cases repo and org where appropriate; ensure comments are written in Swift’s triple-slash format so SwiftLint recognizes them and keep descriptions brief and accurate.Sources/RunnerBar/StepLogView.swift (1)
86-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when the job URL is missing.
job.htmlUrlis now optional, so this fallback is reachable. Defaulting to the first scope can fetch logs from the wrong repo in multi-scope setups; if you cannot deriveowner/repofrom the job itself, return “Log not available” instead of issuing the request with an arbitrary scope.Possible fix
private func loadLog() { isLoading = true let jobID = job.id let stepNum = step.id + guard let htmlUrl = job.htmlUrl else { + logText = "" + isLoading = false + return + } let scope: String = { - let parts = job.htmlUrl?.components(separatedBy: "/") ?? [] + let parts = htmlUrl.components(separatedBy: "/") if parts.count >= 5 { let owner = parts[3] let repo = parts[4] if !owner.isEmpty && !repo.isEmpty { return "\(owner)/\(repo)" } } - return ScopeStore.shared.scopes.first(where: { $0.contains("/") }) ?? "" + return "" }() + guard !scope.isEmpty else { + logText = "" + isLoading = false + return + } DispatchQueue.global(qos: .userInitiated).async { let text = fetchStepLog(jobID: jobID, stepNumber: stepNum, scope: scope) DispatchQueue.main.async {🤖 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 `@Sources/RunnerBar/StepLogView.swift` around lines 86 - 94, The scope computation block that builds `scope` from `job.htmlUrl` (the closure assigning `let scope: String = { ... }()`) currently falls back to `ScopeStore.shared.scopes.first(where:)`, which can pick the wrong repo when `job.htmlUrl` is nil or unparseable; change the fallback so that if `owner/repo` cannot be derived from `job.htmlUrl` you return the sentinel string "Log not available" instead of using `ScopeStore.shared` (so the caller can display a clear message and avoid issuing a request with an arbitrary scope).Sources/RunnerBar/RunnerStoreState.swift (1)
85-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse an ISO-8601 decoder for step backfill.
Line 94 uses a plain
JSONDecoder(), but GitHub's API returns job step timestamps as ISO-8601 formatted strings (e.g.,2019-08-08T08:00:00-07:00). SinceJobPayload.stepsdecodes intoJobStep, whosestartedAtandcompletedAtareDate?, the decoder must be configured withdateDecodingStrategy = .iso8601. Without it, the decode fails andbackfillStepssilently skips enrichment due to thetry?suppression.Suggested fix
private func backfillSteps(into cache: inout [Int: ActiveJob]) { let iso = ISO8601DateFormatter() + let decoder = JSONDecoder() + decoder.dateDecodingStrategy = .iso8601 for cacheID in Array(cache.keys) { guard let cached = cache[cacheID] else { continue } guard cached.conclusion != nil, cached.steps.isEmpty || cached.steps.contains(where: { $0.status == "in_progress" }), let scope = scopeFromHtmlUrl(cached.htmlUrl), let data = ghAPI("repos/\(scope)/actions/jobs/\(cacheID)"), - let fresh = try? JSONDecoder().decode(JobPayload.self, from: data), + let fresh = try? decoder.decode(JobPayload.self, from: data), let rawSteps = fresh.steps, !rawSteps.isEmpty else { continue }🤖 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 `@Sources/RunnerBar/RunnerStoreState.swift` around lines 85 - 98, The JSONDecoder used in backfillSteps fails to parse ISO-8601 timestamps so the try? JSONDecoder().decode(JobPayload.self, from: data) must be replaced with a decoder configured for ISO-8601 dates; create a JSONDecoder, set decoder.dateDecodingStrategy = .iso8601, then use that decoder to decode JobPayload (the variables involved are backfillSteps, ghAPI(...) result, JobPayload, and makeActiveJob) so the JobStep.startedAt/completedAt Date? fields decode properly and the cache gets enriched.
🧹 Nitpick comments (2)
Sources/RunnerBar/SystemStats.swift (1)
100-106: 💤 Low valueRedundant
[weak self]capture in nested closure.Line 104 has a double weak capture: the outer closure already captures
[weak self], makingselfoptional. The inner[weak self]is redundant sinceself?.samplingQueue.asyncwon't execute ifselfis already nil.♻️ Suggested simplification
func start() { timer?.invalidate() samplingQueue.async { [weak self] in self?.sample() } timer = Timer.scheduledTimer(withTimeInterval: 2, repeats: true) { [weak self] _ in - self?.samplingQueue.async { [weak self] in self?.sample() } + self?.samplingQueue.async { self?.sample() } } }🤖 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 `@Sources/RunnerBar/SystemStats.swift` around lines 100 - 106, The nested closure in start() uses a redundant [weak self] capture: keep the outer [weak self] in the Timer closure but remove the inner [weak self] on the samplingQueue.async closure so it becomes samplingQueue.async { self?.sample() }; this applies to the Timer's scheduled closure only (retain the existing [weak self] for the top-level samplingQueue.async and for the Timer closure itself), referencing the start(), timer, samplingQueue, and sample() symbols to locate the change.Sources/RunnerBar/PopoverMainView.swift (1)
74-438: 🏗️ Heavy liftSplit these new subviews out of
PopoverMainView.swift.This file now owns the header, mini bar, actions list, action row, inline jobs, inline job row, and runners list. That makes future layout regressions harder to isolate and directly conflicts with the single-responsibility rule for
Sources/RunnerBar/*.swift.As per coding guidelines, "Keep files small and single-responsibility — add new files rather than growing existing ones".
🤖 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 `@Sources/RunnerBar/PopoverMainView.swift` around lines 74 - 438, Move each subview struct out of PopoverMainView.swift into its own small file under Sources/RunnerBar: MiniBarView.swift (MiniBarView), PopoverHeaderView.swift (PopoverHeaderView), ActionsListView.swift (ActionsListView), ActionRowView.swift (ActionRowView), InlineJobsView.swift (InlineJobsView), InlineJobRowView.swift (InlineJobRowView), and RunnersListView.swift (RunnersListView). For each new file, copy the corresponding struct and any private helpers it uses (e.g., actionStatusLabel/actionStatusColor/actionDotColor in ActionRowView, stepFraction/currentStepTitle/jobDotColor in InlineJobRowView), add necessary imports (import SwiftUI), keep the same access modifiers, and update references in the original PopoverMainView to import/use the moved types. Finally remove the moved definitions from PopoverMainView.swift so the file only contains the main view.
🤖 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 `@Sources/RunnerBar/ActionGroup.swift`:
- Around line 178-179: Remove the unnecessary SwiftLint directive on the
equality operator: delete the comment "// swiftlint:disable:next missing_docs"
that precedes the static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool
implementation in ActionGroup so the superfluous_disable_command violation is
resolved and no lint suppression remains around the == operator.
In `@Sources/RunnerBar/AddRunnerSheet.swift`:
- Around line 173-174: Update the user-facing error string assigned to
errorMessage in AddRunnerSheet (where registration token fetch fails) to remove
the direct GH_TOKEN/PAT prompt and only instruct users to run `gh auth login` or
use the app-managed auth flow; keep the rest of the message context but
eliminate any phrase suggesting users set GH_TOKEN directly so the message only
references `gh auth login` (or app auth) and not token entry.
- Around line 159-163: The prefix check using resolvedDir.hasPrefix(homeDir) is
unsafe (e.g., /Users/alice2), so replace it with a directory-boundary check:
normalize both paths (you already call resolvingSymlinksInPath /
standardizedFileURL), then either compare pathComponents (ensure
resolvedDirURL.pathComponents starts(with: homeDirURL.pathComponents) ) or
require resolvedDir == homeDir OR resolvedDir.hasPrefix(homeDir + "/"); update
the guard around resolvedDir/homeDir in AddRunnerSheet.swift to use one of these
safe checks instead of hasPrefix.
In `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 95-97: The code clears savedNavState inside mainView(), which
causes popoverDidClose() to rebuild the root view and wipe the last
detail/settings state so the restore path never runs; remove the assignment
"savedNavState = nil" from mainView() (or move it to a more appropriate
teardown/initialization location such as a dedicated reset method called only
when you truly want to reset state) so that popoverDidClose() can preserve and
restore savedNavState as intended.
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 57-61: PopoverMainView's new inline action/job rows never call the
job-selection callback, so thread the onSelectJob callback through the view
hierarchy: add an onSelectJob parameter to ActionsListView (in its initializer
and stored property), forward that parameter into each InlineActionRow (or the
row type rendering jobs), and invoke onSelectJob(job) from the row's
tap/selection handler (the same place where onSelectAction is handled) so
selecting an inline job opens JobDetailView; update PopoverMainView,
ActionsListView, and the InlineActionRow (or whatever row struct renders the
inline job) accordingly to pass and call onSelectJob.
- Around line 130-142: The help text for the unauthenticated CTA currently
mentions adding a "GitHub token"; update the .help(...) string to a generic
call-to-action such as "Connect GitHub or open Settings" so the UI doesn't
instruct users to add tokens directly. Locate the Button rendered when
isAuthenticated is false (the Button using onSelectSettings and the .help
modifier) and replace the specific reference to tokens with a generic phrase
that invites connecting GitHub or opening Settings.
In `@Sources/RunnerBar/RunnerStoreState.swift`:
- Around line 170-177: When enriching jobs into ActiveJob (in the code creating
ActiveJob with id/name/status/etc.), preserve a cached htmlUrl by setting
htmlUrl to cached.htmlUrl ?? job.htmlUrl instead of always using job.htmlUrl;
update the ActiveJob construction (the block that currently uses htmlUrl:
job.htmlUrl) to prefer cached.htmlUrl so selected jobs keep their scope/run link
used by JobDetailView and StepLogView.
---
Outside diff comments:
In `@Sources/RunnerBar/AddRunnerSheet.swift`:
- Around line 6-14: Add missing documentation comments for the internal
declarations to satisfy SwiftLint missing_docs: add concise Swift doc comments
(/// ...) above the AddRunnerSheet struct and the nested ScopeType enum
explaining their purpose and usage, and document the properties `@Binding` var
isPresented, let onComplete, and the enum cases repo and org where appropriate;
ensure comments are written in Swift’s triple-slash format so SwiftLint
recognizes them and keep descriptions brief and accurate.
In `@Sources/RunnerBar/RunnerStoreState.swift`:
- Around line 85-98: The JSONDecoder used in backfillSteps fails to parse
ISO-8601 timestamps so the try? JSONDecoder().decode(JobPayload.self, from:
data) must be replaced with a decoder configured for ISO-8601 dates; create a
JSONDecoder, set decoder.dateDecodingStrategy = .iso8601, then use that decoder
to decode JobPayload (the variables involved are backfillSteps, ghAPI(...)
result, JobPayload, and makeActiveJob) so the JobStep.startedAt/completedAt
Date? fields decode properly and the cache gets enriched.
In `@Sources/RunnerBar/StepLogView.swift`:
- Around line 86-94: The scope computation block that builds `scope` from
`job.htmlUrl` (the closure assigning `let scope: String = { ... }()`) currently
falls back to `ScopeStore.shared.scopes.first(where:)`, which can pick the wrong
repo when `job.htmlUrl` is nil or unparseable; change the fallback so that if
`owner/repo` cannot be derived from `job.htmlUrl` you return the sentinel string
"Log not available" instead of using `ScopeStore.shared` (so the caller can
display a clear message and avoid issuing a request with an arbitrary scope).
---
Nitpick comments:
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 74-438: Move each subview struct out of PopoverMainView.swift into
its own small file under Sources/RunnerBar: MiniBarView.swift (MiniBarView),
PopoverHeaderView.swift (PopoverHeaderView), ActionsListView.swift
(ActionsListView), ActionRowView.swift (ActionRowView), InlineJobsView.swift
(InlineJobsView), InlineJobRowView.swift (InlineJobRowView), and
RunnersListView.swift (RunnersListView). For each new file, copy the
corresponding struct and any private helpers it uses (e.g.,
actionStatusLabel/actionStatusColor/actionDotColor in ActionRowView,
stepFraction/currentStepTitle/jobDotColor in InlineJobRowView), add necessary
imports (import SwiftUI), keep the same access modifiers, and update references
in the original PopoverMainView to import/use the moved types. Finally remove
the moved definitions from PopoverMainView.swift so the file only contains the
main view.
In `@Sources/RunnerBar/SystemStats.swift`:
- Around line 100-106: The nested closure in start() uses a redundant [weak
self] capture: keep the outer [weak self] in the Timer closure but remove the
inner [weak self] on the samplingQueue.async closure so it becomes
samplingQueue.async { self?.sample() }; this applies to the Timer's scheduled
closure only (retain the existing [weak self] for the top-level
samplingQueue.async and for the Timer closure itself), referencing the start(),
timer, samplingQueue, and sample() symbols to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d7f9d6c-3143-413b-b8d4-01bdac54341f
📒 Files selected for processing (21)
.gitignore.swiftlint.ymlSources/RunnerBar/ActionDetailView.swiftSources/RunnerBar/ActionGroup.swiftSources/RunnerBar/ActionRunsResponse.swiftSources/RunnerBar/ActiveJob.swiftSources/RunnerBar/AddRunnerSheet.swiftSources/RunnerBar/AppDelegate.swiftSources/RunnerBar/GitHub.swiftSources/RunnerBar/JobDetailView.swiftSources/RunnerBar/LocalRunnerScanner.swiftSources/RunnerBar/LocalRunnerStore.swiftSources/RunnerBar/PieProgressView.swiftSources/RunnerBar/PopoverMainView.swiftSources/RunnerBar/RelativeTimeFormatter.swiftSources/RunnerBar/Runner.swiftSources/RunnerBar/RunnerStoreObservable.swiftSources/RunnerBar/RunnerStoreState.swiftSources/RunnerBar/StepLogView.swiftSources/RunnerBar/SystemStats.swiftSources/RunnerBar/SystemStatsView.swift
Comment on lines
+178
to
+179
| // swiftlint:disable:next missing_docs | ||
| static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool { |
There was a problem hiding this comment.
Remove superfluous SwiftLint disable directive.
SwiftLint reports that missing_docs does not trigger on this == operator function, so the disable:next directive is unnecessary and causes a superfluous_disable_command violation.
🔧 Proposed fix
extension ActionGroup: Equatable {
/// Returns `true` when two groups have the same ID, dimmed state, job list, and run IDs.
- // swiftlint:disable:next missing_docs
static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool {📝 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
| // swiftlint:disable:next missing_docs | |
| static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool { | |
| extension ActionGroup: Equatable { | |
| /// Returns `true` when two groups have the same ID, dimmed state, job list, and run IDs. | |
| static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool { |
🧰 Tools
🪛 GitHub Check: SwiftLint
[failure] 179-179:
SwiftLint rule 'missing_docs' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)
🤖 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 `@Sources/RunnerBar/ActionGroup.swift` around lines 178 - 179, Remove the
unnecessary SwiftLint directive on the equality operator: delete the comment "//
swiftlint:disable:next missing_docs" that precedes the static func == (lhs:
ActionGroup, rhs: ActionGroup) -> Bool implementation in ActionGroup so the
superfluous_disable_command violation is resolved and no lint suppression
remains around the == operator.
Comment on lines
159
to
163
| let homeDir = FileManager.default.homeDirectoryForCurrentUser | ||
| .resolvingSymlinksInPath.path | ||
| .resolvingSymlinksInPath().path | ||
| let resolvedDir = URL(fileURLWithPath: dir) | ||
| .resolvingSymlinksInPath.path | ||
| .resolvingSymlinksInPath().path | ||
| guard resolvedDir.hasPrefix(homeDir) else { |
There was a problem hiding this comment.
Fix home-directory boundary check (prefix check is unsafe).
Using hasPrefix on raw paths can incorrectly allow paths outside home (e.g., /Users/alice2). Use a normalized path with a directory-boundary check.
Suggested fix
- let homeDir = FileManager.default.homeDirectoryForCurrentUser
- .resolvingSymlinksInPath().path
- let resolvedDir = URL(fileURLWithPath: dir)
- .resolvingSymlinksInPath().path
- guard resolvedDir.hasPrefix(homeDir) else {
+ let homeDir = FileManager.default.homeDirectoryForCurrentUser
+ .resolvingSymlinksInPath()
+ .standardizedFileURL
+ .path
+ let resolvedDir = URL(fileURLWithPath: dir)
+ .resolvingSymlinksInPath()
+ .standardizedFileURL
+ .path
+ guard resolvedDir == homeDir || resolvedDir.hasPrefix(homeDir + "/") else {🤖 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 `@Sources/RunnerBar/AddRunnerSheet.swift` around lines 159 - 163, The prefix
check using resolvedDir.hasPrefix(homeDir) is unsafe (e.g., /Users/alice2), so
replace it with a directory-boundary check: normalize both paths (you already
call resolvingSymlinksInPath / standardizedFileURL), then either compare
pathComponents (ensure resolvedDirURL.pathComponents starts(with:
homeDirURL.pathComponents) ) or require resolvedDir == homeDir OR
resolvedDir.hasPrefix(homeDir + "/"); update the guard around
resolvedDir/homeDir in AddRunnerSheet.swift to use one of these safe checks
instead of hasPrefix.
Comment on lines
+173
to
+174
| errorMessage = "Failed to fetch registration token. " | ||
| + "Ensure `gh auth login` has been run or GH_TOKEN is set." |
There was a problem hiding this comment.
Remove direct GH_TOKEN prompting from user-facing copy.
This message asks users to set GH_TOKEN directly, which violates the app policy. Keep the guidance to gh auth login only (or route through app-managed auth flow).
Suggested fix
- errorMessage = "Failed to fetch registration token. "
- + "Ensure `gh auth login` has been run or GH_TOKEN is set."
+ errorMessage = "Failed to fetch registration token. "
+ + "Ensure `gh auth login` has been run."As per coding guidelines “Do not prompt the user for a GitHub token or PAT directly in the app”.
📝 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
| errorMessage = "Failed to fetch registration token. " | |
| + "Ensure `gh auth login` has been run or GH_TOKEN is set." | |
| errorMessage = "Failed to fetch registration token. " | |
| "Ensure `gh auth login` has been run." |
🤖 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 `@Sources/RunnerBar/AddRunnerSheet.swift` around lines 173 - 174, Update the
user-facing error string assigned to errorMessage in AddRunnerSheet (where
registration token fetch fails) to remove the direct GH_TOKEN/PAT prompt and
only instruct users to run `gh auth login` or use the app-managed auth flow;
keep the rest of the message context but eliminate any phrase suggesting users
set GH_TOKEN directly so the message only references `gh auth login` (or app
auth) and not token entry.
Comment on lines
+95
to
97
| @MainActor | ||
| private func mainView() -> AnyView { | ||
| savedNavState = nil |
There was a problem hiding this comment.
Don't clear savedNavState inside mainView().
popoverDidClose() rebuilds the root view with mainView(), so this assignment wipes the last detail/settings state on every close. That makes the restore path in Lines 292-295 unreachable after a normal close/reopen cycle.
Suggested fix
- private func mainView() -> AnyView {
- savedNavState = nil
+ private func mainView(clearSavedState: Bool = true) -> AnyView {
+ if clearSavedState {
+ savedNavState = nil
+ }
return AnyView(PopoverMainView(
store: observable,
onSelectJob: { [weak self] job in
@@
- self.hostingController?.rootView = self.mainView()
+ self.hostingController?.rootView = self.mainView(clearSavedState: false)🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 95 - 97, The code clears
savedNavState inside mainView(), which causes popoverDidClose() to rebuild the
root view and wipe the last detail/settings state so the restore path never
runs; remove the assignment "savedNavState = nil" from mainView() (or move it to
a more appropriate teardown/initialization location such as a dedicated reset
method called only when you truly want to reset state) so that popoverDidClose()
can preserve and restore savedNavState as intended.
Comment on lines
+57
to
+61
| ActionsListView( | ||
| actions: store.actions, | ||
| visibleCount: $visibleCount, | ||
| onSelectAction: onSelectAction | ||
| ) |
There was a problem hiding this comment.
Thread onSelectJob into the new inline rows.
The main view still supports job selection, but the new inline job list renders plain rows and never calls that callback. That turns the new job rows into a dead end and regresses drill-down into JobDetailView from the main popover.
Also applies to: 284-287, 337-374
🤖 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 `@Sources/RunnerBar/PopoverMainView.swift` around lines 57 - 61,
PopoverMainView's new inline action/job rows never call the job-selection
callback, so thread the onSelectJob callback through the view hierarchy: add an
onSelectJob parameter to ActionsListView (in its initializer and stored
property), forward that parameter into each InlineActionRow (or the row type
rendering jobs), and invoke onSelectJob(job) from the row's tap/selection
handler (the same place where onSelectAction is handled) so selecting an inline
job opens JobDetailView; update PopoverMainView, ActionsListView, and the
InlineActionRow (or whatever row struct renders the inline job) accordingly to
pass and call onSelectJob.
Comment on lines
+130
to
+142
| if !isAuthenticated { | ||
| Button( | ||
| action: onSelectSettings, | ||
| label: { | ||
| HStack(spacing: 4) { | ||
| Circle().fill(Color.orange).frame(width: 7, height: 7) | ||
| Text("Sign in").font(.caption2).foregroundColor(.secondary) | ||
| } | ||
| } | ||
| ) | ||
| .buttonStyle(.plain) | ||
| .help("Not authenticated — open Settings to add a GitHub token") | ||
| } |
There was a problem hiding this comment.
Keep the auth CTA generic; don't mention tokens in-app.
The new help text explicitly tells the user to add a GitHub token. That conflicts with the repo rule and couples the UI to a specific credential mechanism; keep this copy at the “connect GitHub / open Settings” level instead.
Possible fix
if !isAuthenticated {
Button(
action: onSelectSettings,
label: {
HStack(spacing: 4) {
Circle().fill(Color.orange).frame(width: 7, height: 7)
Text("Sign in").font(.caption2).foregroundColor(.secondary)
}
}
)
.buttonStyle(.plain)
- .help("Not authenticated — open Settings to add a GitHub token")
+ .help("Not authenticated — open Settings to connect GitHub")
}As per coding guidelines, "Do not prompt the user for a GitHub token or PAT directly in the app".
📝 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
| if !isAuthenticated { | |
| Button( | |
| action: onSelectSettings, | |
| label: { | |
| HStack(spacing: 4) { | |
| Circle().fill(Color.orange).frame(width: 7, height: 7) | |
| Text("Sign in").font(.caption2).foregroundColor(.secondary) | |
| } | |
| } | |
| ) | |
| .buttonStyle(.plain) | |
| .help("Not authenticated — open Settings to add a GitHub token") | |
| } | |
| if !isAuthenticated { | |
| Button( | |
| action: onSelectSettings, | |
| label: { | |
| HStack(spacing: 4) { | |
| Circle().fill(Color.orange).frame(width: 7, height: 7) | |
| Text("Sign in").font(.caption2).foregroundColor(.secondary) | |
| } | |
| } | |
| ) | |
| .buttonStyle(.plain) | |
| .help("Not authenticated — open Settings to connect GitHub") | |
| } |
🤖 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 `@Sources/RunnerBar/PopoverMainView.swift` around lines 130 - 142, The help
text for the unauthenticated CTA currently mentions adding a "GitHub token";
update the .help(...) string to a generic call-to-action such as "Connect GitHub
or open Settings" so the UI doesn't instruct users to add tokens directly.
Locate the Button rendered when isAuthenticated is false (the Button using
onSelectSettings and the .help modifier) and replace the specific reference to
tokens with a generic phrase that invites connecting GitHub or opening Settings.
Comment on lines
+170
to
+177
| return ActiveJob( | ||
| id: job.id, name: job.name, status: job.status, | ||
| conclusion: job.conclusion, | ||
| startedAt: cached.startedAt ?? job.startedAt, | ||
| createdAt: cached.createdAt ?? job.createdAt, | ||
| completedAt: cached.completedAt ?? job.completedAt, | ||
| htmlUrl: job.htmlUrl, isDimmed: job.isDimmed, | ||
| steps: cached.steps |
There was a problem hiding this comment.
Preserve the cached job URL during enrichment.
If the ActionGroup copy of a job is missing htmlUrl, this replacement keeps nil even when jobCache has a populated URL. That leaves the selected job without the scope/run link that JobDetailView and StepLogView rely on.
Possible fix
return ActiveJob(
id: job.id, name: job.name, status: job.status,
conclusion: job.conclusion,
startedAt: cached.startedAt ?? job.startedAt,
createdAt: cached.createdAt ?? job.createdAt,
completedAt: cached.completedAt ?? job.completedAt,
- htmlUrl: job.htmlUrl, isDimmed: job.isDimmed,
+ htmlUrl: cached.htmlUrl ?? job.htmlUrl, isDimmed: job.isDimmed,
steps: cached.steps
)🤖 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 `@Sources/RunnerBar/RunnerStoreState.swift` around lines 170 - 177, When
enriching jobs into ActiveJob (in the code creating ActiveJob with
id/name/status/etc.), preserve a cached htmlUrl by setting htmlUrl to
cached.htmlUrl ?? job.htmlUrl instead of always using job.htmlUrl; update the
ActiveJob construction (the block that currently uses htmlUrl: job.htmlUrl) to
prefer cached.htmlUrl so selected jobs keep their scope/run link used by
JobDetailView and StepLogView.
| case .completed: return 1.0 | ||
| case .inProgress: | ||
| guard jobsTotal > 0 else { return nil } | ||
| return Double(jobsDone) / Double(jobsTotal) |
There was a problem hiding this comment.
Suggestion: The progress calculation uses jobsDone, but jobsDone only counts success and skipped, not other concluded states like failure/cancelled. This makes in-progress groups appear less complete than they really are whenever some jobs have already finished unsuccessfully. Compute the fraction from all concluded jobs (conclusion != nil) so the pie reflects true completion progress. [logic error]
Severity Level: Major ⚠️
- ❌ Actions popover misreports group completion progress.
- ⚠️ Failed jobs make progress pie appear stalled.Steps of Reproduction ✅
1. RunnerStore.fetch() in `RunnerStore.swift:122-149` calls `buildGroupState(...)` in
`RunnerStoreState.swift:1-41`, which iterates `ScopeStore.shared.scopes` and invokes
`fetchActionGroups(for: scope, cache:)` for each scope at `RunnerStoreState.swift:8-11`.
2. `fetchActionGroups(for:scope:)` in `ActionGroup.swift:193-292` builds `ActionGroup`
instances and populates their `jobs` arrays from `ActiveJob` objects returned by
`fetchJobsForRun(...)` at `ActionGroup.swift:324-369`, while also computing `groupStatus`,
`jobsDone`, and `jobsTotal`.
3. In `ActionGroup.swift:90-100`, `groupStatus` is `.inProgress` whenever at least one run
is `"in_progress"` and not all jobs have a non-nil `conclusion`, while `jobsDone` at
`ActionGroup.swift:112-115` counts only jobs where `conclusion == "success" || conclusion
== "skipped"`, ignoring `"failure"` and `"cancelled"` conclusions that are already
finished.
4. For an in-progress group where one job has `conclusion == "success"`, another has
`conclusion == "failure"`, and a third is still running (all jobs present in `jobs`),
`groupStatus` remains `.inProgress`, but `progressFraction` at `ActionGroup.swift:163-170`
computes `Double(jobsDone) / Double(jobsTotal)` (line 169), yielding `1/3` instead of
reflecting that 2 of 3 jobs are concluded; this fraction feeds the `PieProgressView` in
`PopoverMainView.ActionRowView` at `PopoverMainView.swift:35-49`, so the main Actions list
shows a pie that under-represents actual completion whenever failed/cancelled jobs exist
alongside running jobs.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/RunnerBar/ActionGroup.swift
**Line:** 169:169
**Comment:**
*Logic Error: The progress calculation uses `jobsDone`, but `jobsDone` only counts `success` and `skipped`, not other concluded states like `failure`/`cancelled`. This makes in-progress groups appear less complete than they really are whenever some jobs have already finished unsuccessfully. Compute the fraction from all concluded jobs (`conclusion != nil`) so the pie reflects true completion progress.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let steps: [JobStep] = (j.steps ?? []).enumerated().map { idx, s in | ||
| let steps: [JobStep] = (payload.steps ?? []).enumerated().map { idx, step in | ||
| JobStep( | ||
| id: idx + 1, |
There was a problem hiding this comment.
Suggestion: Replacing the API-provided step number with idx + 1 breaks the step-number contract used downstream for log retrieval (step.id is passed as the requested step number). If GitHub returns non-contiguous or shifted step numbers, log fetching will target the wrong section. Preserve the decoded step identifier instead of reindexing. [api mismatch]
Severity Level: Critical 🚨
- ❌ Step log view can show incorrect step logs.
- ⚠️ Debugging specific failing steps becomes unreliable.Steps of Reproduction ✅
1. When the user taps a job in the main popover or Action detail view, `PopoverMainView`
and `ActionDetailView` call their `onSelectJob` closures; `AppDelegate.mainView()` at
`AppDelegate.swift:16-40` and `AppDelegate.actionDetailView(...)` at
`AppDelegate.swift:43-59` handle this by dispatching to a background queue and calling
`enrichStepsIfNeeded(job)` at `AppDelegate.swift:6-14`.
2. `enrichStepsIfNeeded(_:)` fetches a fresh `JobPayload` via
`ghAPI("repos/\(scope)/actions/jobs/\(job.id)")` at `AppDelegate.swift:8-11`, decodes it,
and then calls the global `makeActiveJob(from:iso:isDimmed:)` function defined in
`ActionGroup.swift:296-321`, which rebuilds an `ActiveJob` with a new `steps` array.
3. Inside this global `makeActiveJob`, each decoded `JobStep` (whose `id` maps from the
GitHub `"number"` field via `JobStep.CodingKeys` at `ActiveJob.swift:104-106`) is
discarded and replaced by a new `JobStep` built in the `.enumerated()` closure at
`ActionGroup.swift:299-307`, where `id: idx + 1` (line 301) assigns a purely positional
1-based index instead of preserving the API-provided step number.
4. Later, when the user taps a step in `JobDetailView` at `JobDetailView.swift:107-133`,
`AppDelegate.logViewFromAction(...)` at `AppDelegate.swift:41-51` pushes `StepLogView`
with that `JobStep`; `StepLogView.loadLog()` at `StepLogView.swift:82-96` uses `step.id`
as `stepNumber` and passes it to `fetchStepLog(jobID:stepNumber:scope:)` at
`GitHub.swift:3-55`, which computes `let index = stepNumber - 1` and selects
`sections[index]` from the log (lines 44-52). Because `step.id` was reindexed to `idx + 1`
instead of using the original GitHub step `"number"`, any divergence between the array
position and the server's numbering (e.g. non-contiguous or shifted `"number"` values)
causes `StepLogView` to show the wrong section of the log for the selected step.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/RunnerBar/ActionGroup.swift
**Line:** 301:301
**Comment:**
*Api Mismatch: Replacing the API-provided step number with `idx + 1` breaks the step-number contract used downstream for log retrieval (`step.id` is passed as the requested step number). If GitHub returns non-contiguous or shifted step numbers, log fetching will target the wrong section. Preserve the decoded step identifier instead of reindexing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+76
to
+79
| if let branch = run.headBranch, | ||
| let range = branch.range(of: #"/(\d+)/"#, options: .regularExpression) { | ||
| let digits = branch[range].filter { $0.isNumber } | ||
| return "#\(digits)" |
There was a problem hiding this comment.
Suggestion: The branch fallback treats any digits between slashes as a PR number, which can mislabel normal branches (for example release/version branches) as pull requests. Restrict this fallback to a stricter PR-specific pattern or trusted branch prefixes so non-PR runs are not shown with incorrect #<number> labels. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Non-PR branch runs mislabeled as pull requests.
- ⚠️ Actions popover header shows misleading PR-style labels.Steps of Reproduction ✅
1. `RunnerStore.fetch()` in `RunnerStore.swift:122-149` builds group state by calling
`buildGroupState(...)` in `RunnerStoreState.swift:1-41`, which iterates
`ScopeStore.shared.scopes` and calls `fetchActionGroups(for: scope, cache:)` at
`RunnerStoreState.swift:8-11` to obtain `ActionGroup` instances from live workflow runs.
2. Inside `fetchActionGroups(for:scope:)` at `ActionGroup.swift:231-241`, the
representative run `rep` for each head SHA has its label computed as `let label =
prLabel(from: rep)` (line 236), using `prLabel(from:)` defined in
`ActionRunsResponse.swift:72-81`.
3. `prLabel(from:)` first uses a real pull request if available, but its branch fallback
at `ActionRunsResponse.swift:76-79` searches `run.headBranch` with the regex
`#"/(\d+)/"#`; for a non-PR branch named `release/2024/05`, GitHub sets `headBranch` to
`"release/2024/05"`, the regex matches the `/2024/` segment, `digits` becomes `"2024"`,
and the function returns `"#2024"` even though no PR is attached.
4. The mislabeled `label` is surfaced in the UI: `ActionRowView` in
`PopoverMainView.swift:35-52` renders `Text(actionGroup.label)` as the left-hand
identifier, and `ActionDetailView` at `ActionDetailView.swift:82-87` displays
`group.label` in the header, so branch-based workflow runs for non-PR branches containing
digit segments between slashes are shown with a spurious `#<number>` PR-style label.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/RunnerBar/ActionRunsResponse.swift
**Line:** 76:79
**Comment:**
*Incorrect Condition Logic: The branch fallback treats any digits between slashes as a PR number, which can mislabel normal branches (for example release/version branches) as pull requests. Restrict this fallback to a stricter PR-specific pattern or trusted branch prefixes so non-PR runs are not shown with incorrect `#<number>` labels.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
The previous commit called setFrameSize/contentSize while popover.isShown == true inside navigate(to:), causing the popover to jump position — a known regression explicitly forbidden by the REGRESSION GUARD comments. Reverted navigate(to:) to a pure rootView swap with zero size changes (matches main branch contract). Detail views use .frame(maxWidth:.infinity, maxHeight:.infinity) so they fill the fixed popover frame correctly. ScrollView absorbs any overflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/RunnerBar/AppDelegate.swift (1)
268-268: 💤 Low valueRename
hcto satisfy SwiftLint'sidentifier_namerule.SwiftLint requires variable names between 3-40 characters. Consider renaming to
hostingControllerorcontroller.♻️ Proposed fix
- let hc = hostingController + let controller = hostingController else { return } popoverIsOpen = true observable.reload() - let fitting = hc.view.fittingSize + let fitting = controller.view.fittingSize let width = fitting.width > 0 ? fitting.width : Self.fixedWidth let height = min(fitting.height > 0 ? fitting.height : 300, Self.maxHeight) let size = NSSize(width: width, height: height) - hc.view.setFrameSize(size) + controller.view.setFrameSize(size)🤖 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 `@Sources/RunnerBar/AppDelegate.swift` at line 268, Rename the short variable `hc` to a longer, SwiftLint-compliant name (e.g., `hostingController` or `controller`) where it is declared (`let hc = hostingController`) and update all subsequent uses of `hc` in the surrounding scope to the new identifier so the code compiles and satisfies the `identifier_name` rule; ensure you only change the local binding and its references, not the original `hostingController` property.
🤖 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 `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 62-67: RunnerStore.shared.onChange may be invoked off the main
thread but currently updates statusItem?.button?.image (using
makeStatusIcon(for:)) synchronously; move that UI update onto the main queue so
AppKit access happens on the main thread and keep the existing
DispatchQueue.main.async { self.observable.reload() } behavior. Specifically, in
the onChange closure (RunnerStore.shared.onChange) wrap the call that sets
statusItem?.button?.image in DispatchQueue.main.async (or perform selector on
main) and ensure you still call observable.reload on the main thread as already
done.
---
Nitpick comments:
In `@Sources/RunnerBar/AppDelegate.swift`:
- Line 268: Rename the short variable `hc` to a longer, SwiftLint-compliant name
(e.g., `hostingController` or `controller`) where it is declared (`let hc =
hostingController`) and update all subsequent uses of `hc` in the surrounding
scope to the new identifier so the code compiles and satisfies the
`identifier_name` rule; ensure you only change the local binding and its
references, not the original `hostingController` property.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b899d642-577f-42fa-a708-46e0f07f65b5
📒 Files selected for processing (1)
Sources/RunnerBar/AppDelegate.swift
Comment on lines
62
to
67
| RunnerStore.shared.onChange = { [weak self] in | ||
| guard let self else { return } | ||
| self.statusItem?.button?.image = makeStatusIcon( | ||
| for: RunnerStore.shared.aggregateStatus | ||
| ) | ||
| if !self.popoverIsOpen { self.observable.reload() } | ||
| self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) | ||
| // ⚠️ Always reload — do NOT guard on !popoverIsOpen. | ||
| DispatchQueue.main.async { self.observable.reload() } | ||
| } |
There was a problem hiding this comment.
Main-thread violation: statusItem is updated from the callback's thread context.
RunnerStore.shared.onChange may fire from a background thread, but line 64 updates statusItem?.button?.image synchronously before dispatching to main. AppKit UI elements must be accessed on the main thread.
🐛 Proposed fix: dispatch the status icon update to main thread
RunnerStore.shared.onChange = { [weak self] in
guard let self else { return }
- self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus)
- // ⚠️ Always reload — do NOT guard on !popoverIsOpen.
- DispatchQueue.main.async { self.observable.reload() }
+ // ⚠️ Always reload — do NOT guard on !popoverIsOpen.
+ DispatchQueue.main.async {
+ self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus)
+ self.observable.reload()
+ }
}📝 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
| RunnerStore.shared.onChange = { [weak self] in | |
| guard let self else { return } | |
| self.statusItem?.button?.image = makeStatusIcon( | |
| for: RunnerStore.shared.aggregateStatus | |
| ) | |
| if !self.popoverIsOpen { self.observable.reload() } | |
| self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) | |
| // ⚠️ Always reload — do NOT guard on !popoverIsOpen. | |
| DispatchQueue.main.async { self.observable.reload() } | |
| } | |
| RunnerStore.shared.onChange = { [weak self] in | |
| guard let self else { return } | |
| // ⚠️ Always reload — do NOT guard on !popoverIsOpen. | |
| DispatchQueue.main.async { | |
| self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) | |
| self.observable.reload() | |
| } | |
| } |
🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 62 - 67,
RunnerStore.shared.onChange may be invoked off the main thread but currently
updates statusItem?.button?.image (using makeStatusIcon(for:)) synchronously;
move that UI update onto the main queue so AppKit access happens on the main
thread and keep the existing DispatchQueue.main.async { self.observable.reload()
} behavior. Specifically, in the onChange closure (RunnerStore.shared.onChange)
wrap the call that sets statusItem?.button?.image in DispatchQueue.main.async
(or perform selector on main) and ensure you still call observable.reload on the
main thread as already done.
- Remove maxHeight cap (was truncating content, causing fixed height) - Restore !popoverIsOpen guard in onChange (matches main branch contract) - openPopover() reads fittingSize width+height with NO cap, exactly as main - navigate() stays pure rootView swap, zero size changes (regression guard intact) - PopoverMainView.showInlineJobs: show jobs for inProgress AND queued groups so queued runs with known jobs also show inline rows per spec #178
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/RunnerBar/AppDelegate.swift (2)
77-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
mainView()still erases the state you try to restore on reopen.
popoverDidClose()rebuilds the root view withmainView(), so Line 99 clearssavedNavStatebefore the nextopenPopover()can use Lines 284-286. After a normal close/reopen cycle, the restore path is effectively dead.💡 Minimal fix
-@MainActor -private func mainView() -> AnyView { - savedNavState = nil +@MainActor +private func mainView(clearSavedState: Bool = true) -> AnyView { + if clearSavedState { + savedNavState = nil + } return AnyView(PopoverMainView( store: observable, onSelectJob: { [weak self] job in @@ func popoverDidClose(_ notification: Notification) { popoverIsOpen = false DispatchQueue.main.async { [weak self] in guard let self else { return } - self.hostingController?.rootView = self.mainView() + self.hostingController?.rootView = self.mainView(clearSavedState: false) } }Also applies to: 97-100
🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 77 - 81, popoverDidClose(_:) currently reconstructs the UI by setting hostingController?.rootView = mainView(), which triggers mainView() to clear savedNavState and prevents openPopover() from restoring state; remove (or guard) the rootView reassignment in popoverDidClose(_:) so it does not call mainView(), and instead only clear savedNavState in the explicit reset path (not on close), ensuring savedNavState remains intact for openPopover() to restore; reference popoverDidClose(_:), hostingController?.rootView, mainView(), savedNavState, and openPopover() when making the change.
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the entire
onChangecallback onto the main actor.Line 65 still mutates
NSStatusItembefore any main-thread hop, and Line 68 readspopoverIsOpenfrom the callback’s current thread. The new@unchecked Sendableconformance at Line 33 suppresses the compiler warnings here, but it does not make those AppKit accesses safe.💡 Minimal fix
RunnerStore.shared.onChange = { [weak self] in - guard let self else { return } - self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) - // ⚠️ Guard on !popoverIsOpen — prevents double-reload while popover is open. - // reload() is called explicitly in openPopover() instead. - if !self.popoverIsOpen { - DispatchQueue.main.async { self.observable.reload() } - } + DispatchQueue.main.async { [weak self] in + guard let self else { return } + self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) + // ⚠️ Guard on !popoverIsOpen — prevents double-reload while popover is open. + // reload() is called explicitly in openPopover() instead. + if !self.popoverIsOpen { + self.observable.reload() + } + } }#!/bin/bash set -euo pipefail # Verify whether RunnerStore.onChange can be invoked from a background queue/task. fd -i 'RunnerStore*.swift' . -x sh -c ' for f do echo "== $f ==" rg -n -C3 "\bonChange\b|DispatchQueue\.(main|global)\.async|Task\.detached|MainActor\.run|@MainActor" "$f" done ' sh {}Expected result: any non-main invocation path confirms this callback must hop to the main actor before touching
statusItem,popoverIsOpen, orobservable.Also applies to: 63-70
🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 33 - 39, The onChange callback registered from RunnerStore must perform all AppKit and observable mutations on the main actor: update the closure you pass to RunnerStore.onChange so its body executes on the MainActor (e.g., wrap the current closure body with Task { `@MainActor` in ... } or call await MainActor.run { ... } if the callback is async) before touching statusItem, popover, popoverIsOpen, observable or hostingController; ensure all reads/writes to those symbols occur inside that MainActor hop so no AppKit access happens off the main thread.
🤖 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 `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 272-280: In openPopover(), replace the broken two-line optional
binding with a proper guard (guard let hc = hostingController else { return })
to fix the hc lint error, and normalize spacing around assignments (use single
spaces around = on the width and height lines that reference hc.view.fittingSize
and Self.fixedWidth) so lines using width, height and size follow standard
spacing; keep popoverIsOpen, observable.reload(), and hc.view.setFrameSize(size)
unchanged.
---
Duplicate comments:
In `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 77-81: popoverDidClose(_:) currently reconstructs the UI by
setting hostingController?.rootView = mainView(), which triggers mainView() to
clear savedNavState and prevents openPopover() from restoring state; remove (or
guard) the rootView reassignment in popoverDidClose(_:) so it does not call
mainView(), and instead only clear savedNavState in the explicit reset path (not
on close), ensuring savedNavState remains intact for openPopover() to restore;
reference popoverDidClose(_:), hostingController?.rootView, mainView(),
savedNavState, and openPopover() when making the change.
- Around line 33-39: The onChange callback registered from RunnerStore must
perform all AppKit and observable mutations on the main actor: update the
closure you pass to RunnerStore.onChange so its body executes on the MainActor
(e.g., wrap the current closure body with Task { `@MainActor` in ... } or call
await MainActor.run { ... } if the callback is async) before touching
statusItem, popover, popoverIsOpen, observable or hostingController; ensure all
reads/writes to those symbols occur inside that MainActor hop so no AppKit
access happens off the main thread.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9870da32-1411-4038-9ee6-7705f2f7e7f3
📒 Files selected for processing (2)
Sources/RunnerBar/AppDelegate.swiftSources/RunnerBar/PopoverMainView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/RunnerBar/PopoverMainView.swift
Comment on lines
+272
to
+280
| let hc = hostingController | ||
| else { return } | ||
| popoverIsOpen = true | ||
| observable.reload() | ||
| let fittingWidth = hostingController.view.fittingSize.width | ||
| let size = NSSize( | ||
| width: fittingWidth > 0 ? fittingWidth : Self.fixedWidth, | ||
| height: hostingController.view.fittingSize.height | ||
| ) | ||
| hostingController.view.setFrameSize(size) | ||
| let fitting = hc.view.fittingSize | ||
| let width = fitting.width > 0 ? fitting.width : Self.fixedWidth | ||
| let height = fitting.height > 0 ? fitting.height : 300 | ||
| let size = NSSize(width: width, height: height) | ||
| hc.view.setFrameSize(size) |
There was a problem hiding this comment.
Fix the current SwiftLint failures in openPopover().
CI is already flagging Line 272 (hc) and Line 277 (extra alignment spaces around =), so this hunk will keep failing lint as written.
💡 Minimal fix
- let hc = hostingController
+ let hostingController = hostingController
else { return }
@@
- let fitting = hc.view.fittingSize
- let width = fitting.width > 0 ? fitting.width : Self.fixedWidth
+ let fitting = hostingController.view.fittingSize
+ let width = fitting.width > 0 ? fitting.width : Self.fixedWidth
let height = fitting.height > 0 ? fitting.height : 300
let size = NSSize(width: width, height: height)
- hc.view.setFrameSize(size)
+ hostingController.view.setFrameSize(size)📝 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
| let hc = hostingController | |
| else { return } | |
| popoverIsOpen = true | |
| observable.reload() | |
| let fittingWidth = hostingController.view.fittingSize.width | |
| let size = NSSize( | |
| width: fittingWidth > 0 ? fittingWidth : Self.fixedWidth, | |
| height: hostingController.view.fittingSize.height | |
| ) | |
| hostingController.view.setFrameSize(size) | |
| let fitting = hc.view.fittingSize | |
| let width = fitting.width > 0 ? fitting.width : Self.fixedWidth | |
| let height = fitting.height > 0 ? fitting.height : 300 | |
| let size = NSSize(width: width, height: height) | |
| hc.view.setFrameSize(size) | |
| let hostingController = hostingController | |
| else { return } | |
| popoverIsOpen = true | |
| observable.reload() | |
| let fitting = hostingController.view.fittingSize | |
| let width = fitting.width > 0 ? fitting.width : Self.fixedWidth | |
| let height = fitting.height > 0 ? fitting.height : 300 | |
| let size = NSSize(width: width, height: height) | |
| hostingController.view.setFrameSize(size) |
🧰 Tools
🪛 GitHub Check: SwiftLint
[failure] 277-277:
Operators should be surrounded by a single whitespace when they are being used (operator_usage_whitespace)
[failure] 277-277:
Operators should be surrounded by a single whitespace when they are being used (operator_usage_whitespace)
[failure] 272-272:
Variable name 'hc' should be between 3 and 40 characters long (identifier_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 `@Sources/RunnerBar/AppDelegate.swift` around lines 272 - 280, In
openPopover(), replace the broken two-line optional binding with a proper guard
(guard let hc = hostingController else { return }) to fix the hc lint error, and
normalize spacing around assignments (use single spaces around = on the width
and height lines that reference hc.view.fittingSize and Self.fixedWidth) so
lines using width, height and size follow standard spacing; keep popoverIsOpen,
observable.reload(), and hc.view.setFrameSize(size) unchanged.
- ActionDetailView: remove maxHeight:.infinity — was stretching to main view's height instead of fitting content. fittingSize in AppDelegate.openPopover() now correctly reads each view's own height. - PopoverMainView InlineJobsView: filter to status==in_progress only. Queued jobs must NOT appear as inline rows per spec #178. Only actively running jobs are shown expanded under their action group row.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 150-159: The close button in PopoverMainView currently calls
NSApplication.shared.hide(nil) which hides the whole app; change it to call a
provided onClose callback instead (add an onClose: () -> Void property to
PopoverMainView and invoke it from the Button action) and wire that onClose from
AppDelegate to call popover.close() (use the existing AppDelegate popover
instance and call popover.close() inside the provided closure) so the popover is
dismissed without hiding the app.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e9e5835-e455-4bd4-b8cd-54237c578f30
📒 Files selected for processing (2)
Sources/RunnerBar/ActionDetailView.swiftSources/RunnerBar/PopoverMainView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/RunnerBar/ActionDetailView.swift
Comment on lines
+150
to
+159
| Button( | ||
| action: { NSApplication.shared.hide(nil) }, | ||
| label: { | ||
| Image(systemName: "xmark") | ||
| .font(.system(size: 11)) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| ) | ||
| .buttonStyle(.plain) | ||
| .help("Close popover") |
There was a problem hiding this comment.
Close button hides the app instead of closing the popover.
The help text says "Close popover" but NSApplication.shared.hide(nil) hides the entire application rather than dismissing just the popover. For a menu bar app, consider passing an onClose callback from AppDelegate that calls popover.close() directly.
Suggested approach
struct PopoverHeaderView: View {
let systemStats: SystemStatsViewModel
let isAuthenticated: Bool
let onSelectSettings: () -> Void
+ let onClose: () -> Void
// ...
Button(
- action: { NSApplication.shared.hide(nil) },
+ action: onClose,
label: {
Image(systemName: "xmark")Then wire onClose from PopoverMainView up to AppDelegate where it can call popover.close().
🤖 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 `@Sources/RunnerBar/PopoverMainView.swift` around lines 150 - 159, The close
button in PopoverMainView currently calls NSApplication.shared.hide(nil) which
hides the whole app; change it to call a provided onClose callback instead (add
an onClose: () -> Void property to PopoverMainView and invoke it from the Button
action) and wire that onClose from AppDelegate to call popover.close() (use the
existing AppDelegate popover instance and call popover.close() inside the
provided closure) so the popover is dismissed without hiding the app.
…opover The popover was sized from mainView() fittingSize before show(), then navigate() swapped the rootView without touching size. Detail views always rendered inside main's larger frame. Fix: after navigate(to: restored), read the NEW fittingSize and call setFrameSize + contentSize. This is safe because it happens synchronously before show() returns. navigate() itself still does ZERO size changes (contract preserved for the post-show() navigation path). No changes to inline jobs filter — status==in_progress filter is correct. When all jobs are queued at API level (0/7 in progress), no inline rows is correct behaviour per spec #178.
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
Sources/RunnerBar/AppDelegate.swift (3)
101-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
savedNavState = nilinsidemainView()prevents restoration after close/reopen.
popoverDidClose()callsmainView(), which clearssavedNavState. This makes the restore path inopenPopover()(lines 290-293) unreachable after a normal close.🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 101 - 103, The line clearing savedNavState inside mainView() prevents state restoration; remove the savedNavState = nil from mainView() and instead clear savedNavState explicitly in popoverDidClose() (or only when the user explicitly discards state) so openPopover() can detect and restore the saved state; update mainView(), popoverDidClose(), and ensure openPopover()'s restore path using savedNavState remains reachable.
283-283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwiftLint failures still present: variable name and spacing.
Static analysis reports:
- Line 283:
hcshould be 3-40 characters (identifier_name)- Line 296: Extra whitespace around
=operators (operator_usage_whitespace)💡 Minimal fix
- let hc = hostingController + let hostingCtrl = hostingController else { return } @@ - let fitting = hc.view.fittingSize - let width = fitting.width > 0 ? fitting.width : Self.fixedWidth - let height = fitting.height > 0 ? fitting.height : 300 + let fitting = hostingCtrl.view.fittingSize + let width = fitting.width > 0 ? fitting.width : Self.fixedWidth + let height = fitting.height > 0 ? fitting.height : 300 let size = NSSize(width: width, height: height) - hc.view.setFrameSize(size) + hostingCtrl.view.setFrameSize(size)Also applies to: 295-296
🤖 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 `@Sources/RunnerBar/AppDelegate.swift` at line 283, Rename the short local variable "hc" to a longer, 3–40 character identifier (e.g., hostingControllerRef or hostingViewController) wherever it's declared and used (the declaration currently "let hc = hostingController" and its subsequent uses around the hostingController/lines ~295-296); also fix operator spacing by ensuring a single space on either side of "=" in assignments (e.g., "let hostingControllerRef = hostingController").
67-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMain-thread violation:
statusItem?.button?.imageis still updated off the main queue.Line 69 sets the status bar icon outside any
DispatchQueue.main.asyncblock. SinceRunnerStore.shared.onChangemay fire from a background thread, this remains an AppKit main-thread violation.🐛 Proposed fix
RunnerStore.shared.onChange = { [weak self] in guard let self else { return } - self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) - // ⚠️ Guard on !popoverIsOpen — prevents double-reload while popover is open. - // reload() is called explicitly in openPopover() instead. - if !self.popoverIsOpen { - DispatchQueue.main.async { self.observable.reload() } + // ⚠️ Guard on !popoverIsOpen — prevents double-reload while popover is open. + // reload() is called explicitly in openPopover() instead. + DispatchQueue.main.async { + self.statusItem?.button?.image = makeStatusIcon(for: RunnerStore.shared.aggregateStatus) + if !self.popoverIsOpen { + self.observable.reload() + } } }🤖 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 `@Sources/RunnerBar/AppDelegate.swift` around lines 67 - 75, RunnerStore.shared.onChange's closure updates AppKit UI off the main thread; move the status bar image assignment into the main queue. Inside the onChange closure (the block capturing [weak self]), wrap setting statusItem?.button?.image in DispatchQueue.main.async along with the existing reload call (keeping the !popoverIsOpen guard) so both UI updates run on the main thread; retain the weak self capture and keep reload() being called explicitly from openPopover() as before.
🧹 Nitpick comments (1)
Sources/RunnerBar/AppDelegate.swift (1)
37-37: ⚖️ Poor tradeoffNote:
@unchecked Sendablesilences concurrency warnings but requires manual verification.The class has mutable state (
statusItem,popover,hostingController,savedNavState,popoverIsOpen) accessed across threads. The@MainActorannotations on view factories help, but ensure all call sites respect those annotations. This is acceptable if the threading model has been audited.🤖 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 `@Sources/RunnerBar/AppDelegate.swift` at line 37, The AppDelegate is marked `@unchecked` Sendable but contains mutable UI state (statusItem, popover, hostingController, savedNavState, popoverIsOpen) that must be confined to the main actor; fix by removing `@unchecked` Sendable and annotating the class as `@MainActor` (or annotate the mutable properties and all methods that touch them with `@MainActor`) so all accesses run on the main thread, and update any call sites that interact with AppDelegate to call into MainActor.run or be `@MainActor-aware` to respect the confinement.
🤖 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.
Duplicate comments:
In `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 101-103: The line clearing savedNavState inside mainView()
prevents state restoration; remove the savedNavState = nil from mainView() and
instead clear savedNavState explicitly in popoverDidClose() (or only when the
user explicitly discards state) so openPopover() can detect and restore the
saved state; update mainView(), popoverDidClose(), and ensure openPopover()'s
restore path using savedNavState remains reachable.
- Line 283: Rename the short local variable "hc" to a longer, 3–40 character
identifier (e.g., hostingControllerRef or hostingViewController) wherever it's
declared and used (the declaration currently "let hc = hostingController" and
its subsequent uses around the hostingController/lines ~295-296); also fix
operator spacing by ensuring a single space on either side of "=" in assignments
(e.g., "let hostingControllerRef = hostingController").
- Around line 67-75: RunnerStore.shared.onChange's closure updates AppKit UI off
the main thread; move the status bar image assignment into the main queue.
Inside the onChange closure (the block capturing [weak self]), wrap setting
statusItem?.button?.image in DispatchQueue.main.async along with the existing
reload call (keeping the !popoverIsOpen guard) so both UI updates run on the
main thread; retain the weak self capture and keep reload() being called
explicitly from openPopover() as before.
---
Nitpick comments:
In `@Sources/RunnerBar/AppDelegate.swift`:
- Line 37: The AppDelegate is marked `@unchecked` Sendable but contains mutable UI
state (statusItem, popover, hostingController, savedNavState, popoverIsOpen)
that must be confined to the main actor; fix by removing `@unchecked` Sendable and
annotating the class as `@MainActor` (or annotate the mutable properties and all
methods that touch them with `@MainActor`) so all accesses run on the main thread,
and update any call sites that interact with AppDelegate to call into
MainActor.run or be `@MainActor-aware` to respect the confinement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b31df6f3-ed76-4cd4-a420-423205f3e917
📒 Files selected for processing (1)
Sources/RunnerBar/AppDelegate.swift
AppDelegate: - Call layoutSubtreeIfNeeded() before fittingSize read so SwiftUI has resolved all content heights before we sample. This fixes Settings and ActionDetailView both showing as thin strips (fittingSize was returning near-zero for ScrollView content before layout pass). - Revert nav-restore-before-show pattern (introduced last commit) which raced with observable.reload() causing random crashes on navigation. Restore is back after show(), same as original main branch contract. fittingSize is now correct via layoutSubtreeIfNeeded() so restore order no longer matters for height. LocalRunnerScanner: - scanRunnerJSONFiles() was running `find ~ /opt /usr/local` which traverses all of home including other apps' containers, triggering macOS TCC sandbox permission dialog and crashing on denial. - Scope find to explicit known runner install locations only: ~/actions-runner, ~/.github/runners, ~/runner, /opt/actions-runner These cover 99% of self-hosted runner installs without broad traversal.
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
…mp regression) Root cause of sideways jump: layoutSubtreeIfNeeded() forced an AppKit layout pass on the hosting view at the old frame size, causing AppKit to reposition the popover window horizontally. Explicitly banned by the sizing contract. Correct fix: solve dynamic height in the VIEWS, not AppDelegate. Add .fixedSize(horizontal: false, vertical: true) to the VStack content inside each ScrollView. This tells SwiftUI the content wants its ideal height, making fittingSize return the correct value without any AppKit layout pass. AppDelegate.openPopover() reads fittingSize exactly as before, no new calls added. AppDelegate: - Remove layoutSubtreeIfNeeded() — the cardinal sin causing the jump - Restore sizing contract comment; add explicit ban on layoutSubtreeIfNeeded ActionDetailView: - Add .fixedSize(horizontal: false, vertical: true) to VStack inside ScrollView - Update regression guard comment JobDetailView: - Add .fixedSize(horizontal: false, vertical: true) to VStack inside ScrollView - Remove .maxHeight:.infinity from root frame — violated contract (caused view to stretch into main view's pre-existing frame) SettingsView: - Add .fixedSize(horizontal: false, vertical: true) to VStack inside ScrollView
…ot frames Removing it caused the sideways jump on navigate() — detail views are shown via rootView swap AFTER show(), so they must fill the existing popover frame. Without maxHeight:.infinity the frame mismatch triggers AppKit to reposition the popover window horizontally.
|
eonist
force-pushed
the
main
branch
2 times, most recently
from
f41d3ab to
310551a
Compare
eonist
changed the title
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.
CodeAnt-AI Description
Show more actions and job details in the popover without layout jumps
What Changed
Impact
✅ Easier browsing of long action histories✅ Fewer popover size jumps✅ Fewer macOS permission prompts when detecting local runners🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.