feat: PopoverMainView redesign (#296) (design-branch-2)#371
Closed
eonist wants to merge 80 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.
The minHeight=360 added in 4c5d8a3 violated the regression guard: Size is set ONCE per open in openPopover() from fittingSize, capped at maxHeight. This caused the popover to open at 360px on a 1-row main view, producing visible jumping and empty space. Restored to exact pre-4c5d8a3 state.
The popover was being sized from a stale fittingSize — SwiftUI had not yet processed the observable.reload() published changes when fittingSize was read, so the measurement reflected the previous close state (often a 1-row main view at ~80px). ActionDetailView's ScrollView was then crammed into that tiny frame, cutting off all job rows. Fix: call layoutSubtreeIfNeeded() after reload() so AppKit flushes the SwiftUI layout pass before we measure fittingSize. This is the correct, safe way to get an accurate height — no minHeight floor, no size changes after show(), no violations of the sizing contract.
…ontract The feature branch removed the `if !self.popoverIsOpen` guard from onChange, allowing RunnerStore polls to fire reload() while the popover is open. This caused @published changes to trigger SwiftUI re-renders against a frame already sized at open time, corrupting layout on every poll cycle — jobs disappeared, detail got clipped, main shrank. Main branch has always guarded reload() behind !popoverIsOpen. The PopoverMainView contract (❌ NO ScrollView, fittingSize measured once) requires data to be frozen while the popover is open. Restore that guard exactly as main branch does it. Also removes the erroneous layoutSubtreeIfNeeded() call added in the previous commit — that was treating a symptom, not the cause.
…before ActionDetailView 1. openPopover() defers size-measure + show to next runloop tick so SwiftUI has processed @published changes from reload() before fittingSize is read. Fixes main view cut-off on open. 2. onSelectAction now enriches group.jobs on a background thread (same pattern as onSelectJob/enrichStepsIfNeeded) before navigating to ActionDetailView. Fixes 'No jobs available' / 0/0 jobs when a workflow just started and jobs array was empty at tap time.
…queued jobs in active group 1. AppDelegate.navigate() now resizes the popover to the new view's fittingSize before swapping rootView, so detail views fit their content instead of inheriting the main view's height. 2. ActionDetailView.jobStatusLabel returns 'In Progress' for queued jobs when the parent group is in progress, matching spec #178.
…dth up to 540 1. InlineJobsView filter changed from 'conclusion == nil' (all non-done) to 'status == in_progress' only. Per spec #178 active mode: only jobs that are actively running appear as inline child rows. Queued jobs that haven't started yet are NOT shown inline — they have no step data and no meaningful progress to display. 2. AppDelegate maxWidth raised to 540, fixedWidth becomes idealWidth fallback. fittingSize drives actual width so content sets its own width up to the cap, reducing truncation.
… pushed out of view Splits PopoverMainView into a pinned header block (system stats + runners + rate-limit banner) and a ScrollView body for the actions list. The ScrollView is capped at maxBodyHeight (maxHeight - headerHeight = 500) so it never overflows the popover window. The header is always visible. Removes the no-ScrollView restriction from the regression guard — that rule applied to fittingSize-driven sizing, which is no longer needed for the scrollable section since its height is fixed by the cap.
… inline jobs re-render on actions update - Root frame: .frame(idealWidth: 420, alignment: .top) only — NO maxWidth: .infinity. maxWidth: .infinity caused fittingSize.width to return the window width (~540) instead of content width (~420), making navigate() resize the popover and shift it sideways. - onChange(of: store.actions): only reset visibleCount when it has been paged beyond default (>10) so poll-driven job enrichment updates (empty→populated jobs) don't disrupt inline job rendering. ActionRowView already re-evaluates inlineJobs reactively from the new actions value passed down through the ForEach — no extra state needed. - Update REGRESSION GUARD comment: correct RULE 1 to match actual correct frame contract.
… side-jump + inline jobs) AppDelegate.navigate() was rewritten on this branch to call setFrameSize + contentSize on every navigation. That is the direct cause of the side-jump: the popover resizes on every rootView swap and shifts its anchor position. main branch is the proof: navigate() does ZERO sizing — it is a rootView swap only. Restored exactly. openPopover() now defers fittingSize read one async tick (matching main's safe contract) so SwiftUI has a full layout pass before we set contentSize. PopoverMainView: removed the ScrollView wrapper around ActionsListView. The ScrollView with .frame(maxHeight:500) clips inline job rows that haven't been laid out yet at the time fittingSize is read, causing them to appear empty. Without ScrollView the popover sizes naturally to its content via fittingSize at open — exactly as main does. Root frame restored to .frame(idealWidth: 420, maxWidth: .infinity, alignment: .top) matching main branch contract (maxWidth:.infinity is correct when navigate() never resizes — it does not affect fittingSize when there is no active resize pass). Pagination: visibleCount reset only when >10, preserving inline job rows on poll ticks.
…es mis-positioned popover) fittingSize must be read synchronously before pop.show() so the popover anchors correctly to the status bar button. Wrapping in DispatchQueue.main.async caused pop.show() to fire with the wrong contentSize, positioning the popover at the top-left of the screen instead of below the menu bar icon. Restored to match main branch contract exactly: 1. fittingSize read synchronously 2. setFrameSize + contentSize set before pop.show() 3. pop.show() called immediately after in the same block 4. navigate() remains a zero-size rootView swap only, forever. ❌ NEVER wrap openPopover() sizing in DispatchQueue.main.async
…e exactly The @mainactor annotation on openPopover() causes the method to hop to the main actor asynchronously when called from the @objc togglePopover(), breaking the synchronous sizing contract. pop.show() fires before contentSize is set, anchoring the popover at the wrong screen position. Fix: match main branch class structure exactly: - final class NSObject (no @unchecked Sendable) - private let observable (no @mainactor lazy var) - no @mainactor on any view factory - no @mainactor on openPopover() or togglePopover() Feature-specific additions retained: idealWidth 420, maxWidth/maxHeight caps, enrichGroupIfNeeded, async action group enrichment.
#370) Under Architecture 1 (sizingOptions = .preferredContentSize), an uncapped ScrollView reports its full scroll content height as the SwiftUI ideal height. NSHostingController publishes this as preferredContentSize.height, causing NSPopover to re-anchor from scratch on every navigation — side-jump. Fix: add .frame(maxHeight: NSScreen.main.map { $0.visibleFrame.height * 0.75 } ?? 600) to the ScrollView in each of the three drill-down views. Files changed: - ActionDetailView.swift — ScrollView wrapping jobs ForEach - JobDetailView.swift — ScrollView wrapping steps ForEach - StepLogView.swift — ScrollView wrapping log Text Architecture 1 contract preserved: - sizingOptions = .preferredContentSize unchanged - No contentSize / setFrameSize calls added - PopoverMainView root .frame(idealWidth: 420) unchanged - onLogLoaded / two-hop mechanism removed (not needed under Arch 1)
|
CodeAnt AI is reviewing your PR. |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
📝 WalkthroughWalkthroughThis PR redesigns PopoverMainView and related infrastructure to implement issue ChangesPopoverMainView Redesign & Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| } | ||
| ) | ||
| .buttonStyle(.plain) | ||
| .help("Not authenticated — open Settings to add a GitHub token") |
There was a problem hiding this comment.
Suggestion: Update this authentication help text to instruct users to run gh auth login (with optional GH_TOKEN/GITHUB_TOKEN fallback) instead of telling them to add a GitHub token directly. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The repository guidance indicates authentication should be handled via gh auth login with token env var fallbacks, not by directing users to add a GitHub token manually in this tooltip. The existing help text violates that guidance, so the suggestion is a real match.
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:** 167:167
**Comment:**
*Custom Rule: Update this authentication help text to instruct users to run `gh auth login` (with optional `GH_TOKEN`/`GITHUB_TOKEN` fallback) instead of telling them to add a GitHub token directly.
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
+321
to
329
| private func actionStatusColor(for group: ActionGroup) -> Color { | ||
| switch group.groupStatus { | ||
| case .inProgress: return .yellow | ||
| case .queued: return .blue | ||
| case .completed: | ||
| if group.isDimmed { return .secondary } | ||
| return group.conclusion == "success" ? .green : .red | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Completed actions are colored red for every non-success conclusion, which incorrectly marks cancelled, skipped, or unknown conclusions as failures. This is inconsistent with the rest of the UI status-color mapping and will mislead users about run outcomes. Map only actual failures to red and use a neutral color for non-failure conclusions. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Completed cancelled actions appear as failures in main list.
- ⚠️ UI color semantics inconsistent between list and detail views.Steps of Reproduction ✅
1. In `ActionDetailView` at `Sources/RunnerBar/ActionDetailView.swift:13-28`, verify the
status-color contract: `conclusionLabel(_:)` and `conclusionColor(_:)` map `"success"` to
a green label, `"failure"` to red, and treat other conclusions such as `"cancelled"` and
`"skipped"` as non-failures, coloring them `.secondary` (neutral) via `conclusionColor`.
2. In `ActionRowView.actionStatusLabel(for:)` at
`Sources/RunnerBar/PopoverMainView.swift:306-319`, see that when `group.groupStatus ==
.completed`, the label becomes `"SUCCESS"`, `"FAILED"`, `"CANCELED"`, `"SKIPPED"`, or
`"DONE"` based on `group.conclusion`, so `"CANCELED"` and `"SKIPPED"` are treated as
distinct non-success labels.
3. In `ActionRowView.actionStatusColor(for:)` at
`Sources/RunnerBar/PopoverMainView.swift:321-329`, observe the color logic: `.yellow` for
`.inProgress`, `.blue` for `.queued`, and for `.completed` it returns `.secondary` only
when `group.isDimmed` is true, otherwise `group.conclusion == "success" ? .green : .red`,
which colors every non-success conclusion (`"failure"`, `"cancelled"`, `"skipped"`, or
unknown) as `.red`.
4. Run the app with a completed action whose `ActionGroup.conclusion` is `"cancelled"` or
`"skipped"` (for example, cancel a GitHub Actions run so it appears in
`RunnerStoreObservable.actions`), open the main popover, and observe that the
corresponding row in `ActionRowView` shows status text `"CANCELED"` or `"SKIPPED"` colored
red via `actionStatusColor(for:)`, while navigating into the detailed jobs view uses
`conclusionColor(_:)` from `ActionDetailView.swift:23-28`, which renders the same
conclusions with neutral `.secondary` color, confirming that non-failure states are
incorrectly rendered as failures in the main list.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:** 321:329
**Comment:**
*Incorrect Condition Logic: Completed actions are colored red for every non-success conclusion, which incorrectly marks `cancelled`, `skipped`, or unknown conclusions as failures. This is inconsistent with the rest of the UI status-color mapping and will mislead users about run outcomes. Map only actual failures to red and use a neutral color for non-failure conclusions.
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| /// Container for inline ↳ job sub-rows. Receives ONLY in_progress jobs. | ||
| private struct InlineJobsView: View { | ||
| let jobs: [ActiveJob] | ||
| @State private var cap: Int = 4 |
There was a problem hiding this comment.
Suggestion: The inline-jobs display cap is initialized to 4, but the redesign contract in this PR states inline jobs should be capped at 3 in the view layer. This causes one extra row to render by default and violates the intended pagination behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Inline jobs section shows four rows instead of three.
- ⚠️ Visual pagination behavior diverges from documented redesign spec.Steps of Reproduction ✅
1. The PR description (provided with this review) states that display caps are enforced in
the view layer as "10 actions, 3 inline jobs", and the top-of-file INLINE JOBS SPEC
comments in `Sources/RunnerBar/PopoverMainView.swift:27-31` confirm that inline jobs are a
view-layer concern, implying a bounded number of inline job rows per action group.
2. In `ActionRowView.inlineJobs` at `Sources/RunnerBar/PopoverMainView.swift:255-258`,
inline jobs are defined as `actionGroup.jobs` filtered to those with `status ==
"in_progress"` when `groupStatus == .inProgress`, and these are passed into
`InlineJobsView(jobs: inlineJobs)` at `lines 300-302`.
3. Inspect `InlineJobsView` at `Sources/RunnerBar/PopoverMainView.swift:344-366`: it
declares `@State private var cap: Int = 4` at line 347 and then, in its body at lines
349-352, renders inline rows with `ForEach(jobs.prefix(cap)) { job in
InlineJobRowView(job: job) }`, so by default up to four inline jobs are shown for a single
action group.
4. Run the app with any `ActionGroup` where there are at least four `ActiveJob` entries
satisfying the inline filter (group in progress, jobs with `status == "in_progress"`);
when the popover renders, `InlineJobsView` shows four inline job rows for that group
before the "+ N more jobs…" pagination button appears, exceeding the intended cap of three
inline jobs specified for the redesign.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:** 347:347
**Comment:**
*Logic Error: The inline-jobs display cap is initialized to 4, but the redesign contract in this PR states inline jobs should be capped at 3 in the view layer. This causes one extra row to render by default and violates the intended pagination behavior.
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
+350
to
+351
| ForEach(jobs.prefix(cap)) { job in | ||
| InlineJobRowView(job: job) |
There was a problem hiding this comment.
Suggestion: Inline job rows are rendered as plain views, not tappable controls, so users cannot open a job directly from the main popover anymore. This breaks the existing job-detail navigation flow that AppDelegate still wires for main-view job selection. Wrap each inline row in a button and forward selection through the existing callback path. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Main popover cannot open job detail from inline runs.
- ⚠️ AppDelegate.mainView onSelectJob callback path becomes unused.Steps of Reproduction ✅
1. In `AppDelegate.mainView()` at `Sources/RunnerBar/AppDelegate.swift:103-133`, observe
that `PopoverMainView` is constructed with an `onSelectJob: (ActiveJob) -> Void` callback
which enriches the job and navigates to `detailView(job:)` via `navigate(to:)` when
invoked.
2. In `PopoverMainView` at `Sources/RunnerBar/PopoverMainView.swift:45-53`, note that the
view stores `let onSelectJob: (ActiveJob) -> Void` but, in its body (`lines 55-98`), this
callback is never passed down into any subview (`ActionsListView`, `ActionRowView`,
`InlineJobsView` or `InlineJobRowView`).
3. Inspect the inline jobs stack: `ActionRowView` at
`Sources/RunnerBar/PopoverMainView.swift:248-303` computes `inlineJobs` and, when
non-empty, renders `InlineJobsView(jobs: inlineJobs)` at `lines 300-302`; `InlineJobsView`
at `lines 344-366` then renders each job with `ForEach(jobs.prefix(cap)) { job in
InlineJobRowView(job: job) }` (lines 350-351), and `InlineJobRowView` at `lines 370-403`
is just a plain `HStack` with no `Button` or gesture and no way to call `onSelectJob`.
4. Run the app so that there is at least one `ActionGroup` where `groupStatus ==
.inProgress` and a job where `status == "in_progress"` (these appear inline per
`inlineJobs` at lines 255-258), open the popover, and click on an inline job row; the
click does nothing because the row is not wrapped in a `Button`, and the `onSelectJob`
path wired in `AppDelegate.mainView()` is never invoked, so job-detail navigation from the
main popover is effectively broken.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:** 350:351
**Comment:**
*Incomplete Implementation: Inline job rows are rendered as plain views, not tappable controls, so users cannot open a job directly from the main popover anymore. This breaks the existing job-detail navigation flow that `AppDelegate` still wires for main-view job selection. Wrap each inline row in a button and forward selection through the existing callback path.
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
+170
to
+178
| 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.
Suggestion: The enrichment path merges cached timestamps/steps into group jobs but keeps status and conclusion from the stale job object. When the group fetch lags behind and jobCache has fresher completed data, this creates inconsistent job state (for example completed timings/steps paired with non-completed status), which can produce incorrect progress/status rendering. Use the cached job's lifecycle fields when taking cached step data. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ ActionDetailView job rows can show finished progress as in-progress.
- ⚠️ Popover inline jobs may show mismatched status and step progress.
- ⚠️ Users may misread workflow completion state from inconsistent UI.Steps of Reproduction ✅
1. Launch the app so `AppDelegate.applicationDidFinishLaunching` sets up the popover and
starts polling via `RunnerStore.shared.start()`
(Sources/RunnerBar/AppDelegate.swift:48–72), which in turn calls `RunnerStore.fetch()` on
a timer (Sources/RunnerBar/RunnerStore.swift:3–18).
2. During a poll, `RunnerStore.fetch()` builds job and group state: it calls
`buildJobState(snapPrev:snapCache:)` and then
`buildGroupState(snapPrevGroups:snapGroupCache:jobCache:)`
(Sources/RunnerBar/RunnerStore.swift:3–18). `buildJobState` creates `jobResult.newCache`
containing completed jobs, and `backfillSteps(into:)` enriches those cache entries with
fresh timestamps and full steps from the GitHub job API
(Sources/RunnerBar/RunnerStoreState.swift:31–100).
3. In the same poll, `buildGroupState` fetches action groups and their jobs via
`fetchActionGroups(for:cache:)` (Sources/RunnerBar/RunnerStoreState.swift:121–131,
Sources/RunnerBar/ActionGroup.swift:28–63). Because `jobCache` is persisted across polls
and enriched via `backfillSteps`, there are realistic moments where a given job ID appears
in `jobCache` as completed with full steps while the corresponding `ActionGroup.jobs`
entry still has a stale lifecycle (`status == "in_progress"`, `conclusion == nil`, and
steps empty or containing an `"in_progress"` step).
4. For such a job, `buildGroupState` calls `enrichGroupJobs(_:jobCache:)`
(Sources/RunnerBar/RunnerStoreState.swift:162–180). The guard at lines 165–169 passes
because there is a cached entry with non-empty steps and the group job has empty or
in-progress steps. The function then constructs a new `ActiveJob` using stale lifecycle
fields from the group job (`status: job.status`, `conclusion: job.conclusion`) but mixes
in timestamps and steps from the cached job (`startedAt/createdAt/completedAt` and `steps:
cached.steps`) (Sources/RunnerBar/RunnerStoreState.swift:170–177). This produces an
inconsistent object—for example, `status == "in_progress"` and `conclusion == nil` paired
with a non-nil `completedAt` and all steps concluded.
5. `buildGroupState` wraps these jobs into `GroupPollResult.display` and assigns them to
`RunnerStore.actions` (Sources/RunnerBar/RunnerStoreState.swift:145–159,
Sources/RunnerBar/RunnerStore.swift:19–26). When the user opens an action group,
`AppDelegate.actionDetailView(group:)` and `ActionDetailView` render that job list
(Sources/RunnerBar/AppDelegate.swift:135–153,
Sources/RunnerBar/ActionDetailView.swift:134–188). In `ActionDetailView`,
`jobStatusLabel(for:)` and `jobStatusColor(for:)` use `job.status`/`job.conclusion` to
show "In Progress" with a yellow dot (Sources/RunnerBar/ActionDetailView.swift:212–239),
while `job.elapsed` and `progressFraction` (Sources/RunnerBar/ActiveJob.swift:32–62) use
the cached `completedAt` and fully-concluded `steps` to display a finished duration and
100% progress, exposing the contradictory stale lifecycle state in the UI.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/RunnerStoreState.swift
**Line:** 170:178
**Comment:**
*Incomplete Implementation: The enrichment path merges cached timestamps/steps into group jobs but keeps `status` and `conclusion` from the stale job object. When the group fetch lags behind and `jobCache` has fresher completed data, this creates inconsistent job state (for example completed timings/steps paired with non-completed status), which can produce incorrect progress/status rendering. Use the cached job's lifecycle fields when taking cached step data.
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
+77
to
+83
| // ⚠️ NO ScrollView — let the VStack grow naturally so fittingSize captures | ||
| // all inline job rows. AppDelegate caps height at 620pt. | ||
| ActionsListView( | ||
| actions: store.actions, | ||
| visibleCount: $visibleCount, | ||
| onSelectAction: onSelectAction | ||
| ) |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
ActionsListView is no longer wrapped in a ScrollView while the popover height is hard-capped at 620pt in AppDelegate, so when users load enough actions (via "Load 10 more actions…") that the content exceeds 620pt, rows past the cap become unreachable.
Suggestion: Reintroduce a bounded vertical ScrollView around the actions region (keeping the sticky header outside it) so overflowing action rows and inline jobs can be scrolled within the popover while still respecting the 620pt height cap in AppDelegate.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** Sources/RunnerBar/PopoverMainView.swift
**Line:** 77:83
**Comment:**
*HIGH: ActionsListView is no longer wrapped in a ScrollView while the popover height is hard-capped at 620pt in AppDelegate, so when users load enough actions (via "Load 10 more actions…") that the content exceeds 620pt, rows past the cap become unreachable.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
+167
to
+169
| case .inProgress: | ||
| guard jobsTotal > 0 else { return nil } | ||
| return Double(jobsDone) / Double(jobsTotal) |
There was a problem hiding this comment.
Suggestion: The completion fraction uses jobsDone, but jobsDone only counts success/skipped jobs and excludes concluded failure/cancelled jobs. This makes progress stay artificially low during runs that already have failed/cancelled completed jobs. Compute the fraction from all concluded jobs (for example conclusion != nil) so the pie reflects real completion. [incorrect variable usage]
Severity Level: Major ⚠️
- ❌ Popover action progress undercounts when jobs fail or cancel.
- ⚠️ Action detail header misreports concluded job count string.
- ⚠️ Progress UI inconsistent with job-level step completion logic.Steps of Reproduction ✅
1. Open the popover via the status bar icon, which triggers `openPopover()` in
`Sources/RunnerBar/AppDelegate.swift:48-71`. That calls `observable.reload()` at line 57,
which in turn copies `RunnerStore.shared.actions` into `RunnerStoreObservable.actions` in
`Sources/RunnerBar/RunnerStoreObservable.swift:33-40`.
2. `RunnerStore.shared.fetch()` in `Sources/RunnerBar/RunnerStore.swift:122-151` populates
`actions` by calling `buildGroupState` (lines 121-159 in `RunnerStoreState.swift`), which
calls `fetchActionGroups(for:cache:)` at `Sources/RunnerBar/ActionGroup.swift:184-255`.
This constructs each `ActionGroup` with its `jobs` array of `ActiveJob` values decoded in
`ActiveJob.swift:5-27`, where `conclusion` can be `"success"`, `"failure"`, `"cancelled"`,
etc. (lines 11-14).
3. For any in-progress action group where at least one job has finished with a non-success
outcome (e.g., a job with `status == "completed"` and `conclusion == "failure"` or
`"cancelled"` in `ActiveJob`), the `jobsDone` property in
`Sources/RunnerBar/ActionGroup.swift:112-115` counts only jobs whose `conclusion` is
`"success"` or `"skipped"`, contrary to its docstring "Number of jobs with a concluded
result across all sibling runs."
4. In the popover UI, `ActionsListView` in
`Sources/RunnerBar/PopoverMainView.swift:205-219` renders each `ActionGroup` using
`ActionRowView`. Inside `ActionRowView.body` at lines 41-51 (same file), `PieProgressView`
is created with `progress: actionGroup.progressFraction`. For `.inProgress` groups,
`progressFraction` at `Sources/RunnerBar/ActionGroup.swift:163-170` returns
`Double(jobsDone) / Double(jobsTotal)`, so completed `"failure"`/`"cancelled"` jobs are
not counted as done. The user sees a pie-progress dot that under-reports completion
whenever there are failed/cancelled jobs, even though those jobs have finished and are
included in `jobsTotal`.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:** 167:169
**Comment:**
*Incorrect Variable Usage: The completion fraction uses `jobsDone`, but `jobsDone` only counts `success`/`skipped` jobs and excludes concluded `failure`/`cancelled` jobs. This makes progress stay artificially low during runs that already have failed/cancelled completed jobs. Compute the fraction from all concluded jobs (for example `conclusion != nil`) so the pie reflects real completion.
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
Sources/RunnerBar/ActionGroup.swift (1)
173-175: ⚡ Quick winPrefer synthesized
Equatableover this partial comparator.This
==ignores fields that still affect behavior and presentation (status,conclusion,title, timestamps, repo, etc.), so it creates a footgun for any future equality-based diffing or tests. MakingWorkflowRunRefequatable and letting Swift synthesizeActionGroupequality is safer than keeping a hand-rolled subset here.🤖 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 173 - 175, The custom Equatable implementation on ActionGroup (the static func == in ActionGroup) only compares a subset of fields and should be replaced by the synthesized Equatable; to do that, make WorkflowRunRef (the element type of ActionGroup.runs) conform to Equatable (implement/derive Equatable for WorkflowRunRef by adding conformance and ensuring its equality covers status, conclusion, title, timestamps, repo, id, etc.), then remove the hand-written static func == from ActionGroup so Swift can synthesize full-value equality for ActionGroup.runs and all other stored properties..swiftlint.yml (1)
42-44: ⚡ Quick winKeep the stricter
file_lengthguardrail.Raising this warning to 500 weakens the repo-wide check that pushes
Sources/RunnerBar/*.swiftback toward smaller, single-responsibility files. Prefer splitting the oversized files or using a narrowly scoped suppression instead of relaxing the threshold globally.As per coding guidelines,
Sources/RunnerBar/*.swift: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 @.swiftlint.yml around lines 42 - 44, Revert the global file_length relaxation in .swiftlint.yml and restore the stricter warning threshold (keep the original lower warning value) instead of setting warning: 500, and for any legitimately large Sources/RunnerBar/*.swift files add a narrow, file-scoped suppression (e.g., add a top-of-file // swiftlint:disable file_length and re-enable after the block) or split the file into smaller units; target the file_length rule in .swiftlint.yml and the specific Sources/RunnerBar/*.swift files when applying fixes.Sources/RunnerBar/PopoverMainView.swift (1)
100-452: 🏗️ Heavy liftSplit the new subviews out of
PopoverMainView.swift.This file now owns layout constants, header rendering, stat chips, actions pagination, inline jobs, and runners rendering. That makes the redesign much harder to reason about and breaks the RunnerBar file-size/single-responsibility rule.
As per coding guidelines,
Sources/RunnerBar/*.swift: 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 100 - 452, The PopoverMainView file has grown too large—extract the new subviews (MiniBarView, PopoverHeaderView including its statChip helper, ActionsListView, ActionRowView and its helpers actionStatusLabel/actionStatusColor/actionDotColor, InlineJobsView, InlineJobRowView, and RunnersListView) into one or more small SwiftUI source files so each type has a single responsibility; for each extracted struct create a new file with import SwiftUI, preserve their current access level (private if they are only used by PopoverMainView, otherwise internal), copy any small helper methods used by that view (e.g., statChip, actionStatusLabel, stepFraction, currentStepTitle) into the same file, update references in PopoverMainView to the moved types, and run the build to fix any visibility or module import issues.
🤖 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 157-169: The progressFraction currently uses jobsDone as the
numerator which omits failed and cancelled jobs; change it to use a "concluded"
count (e.g. concluded = jobsDone + jobsFailed + jobsCancelled) so
progressFraction returns Double(concluded)/Double(jobsTotal) (guarding jobsTotal
> 0), and update any related consumer (such as jobProgress display logic) to use
that same concluded count so the pie and text remain consistent; locate and
modify the progressFraction computed property and the jobProgress rendering to
reference this concluded calculation (keep the .queued -> nil and .completed ->
1.0 branches unchanged).
In `@Sources/RunnerBar/AddRunnerSheet.swift`:
- Around line 159-167: The current boundary check using
resolvedDir.hasPrefix(homeDir) is unsafe; update the guard that compares homeDir
and resolvedDir (computed from FileManager.default.homeDirectoryForCurrentUser
and URL(fileURLWithPath: dir).resolvingSymlinksInPath().path) to ensure a true
directory-boundary match — e.g. normalize both to standardized/resolved URLs or
paths and check that resolvedDir equals homeDir or begins with homeDir + "/" (or
compare pathComponents to ensure homeDir's components are a prefix of
resolvedDir's components) before allowing the install path; adjust the guard and
keep the existing DispatchQueue.main.async error handling for the failing case.
In `@Sources/RunnerBar/AppDelegate.swift`:
- Around line 279-280: Normalize operator spacing in the clamp expressions for
width and height in AppDelegate (variables: width, height) so SwiftLint passes:
remove extra spaces and ensure single spaces around binary operators and the
ternary components (fittingWidth, fittingHeight, the ">" comparison, the "?" and
":" parts, and the commas separating arguments) when referencing
Self.idealWidth, Self.minHeight, Self.maxWidth, etc.; update the expressions
that use max(...), min(...) and the ternary checks (fittingWidth > 0 ?
fittingWidth : Self.idealWidth and fittingHeight > 0 ? fittingHeight : 300) to
have consistent single-space operator spacing.
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 77-83: The ActionsListView is being added directly to the VStack
which causes content to be clipped by the popover height cap; wrap the paged
actions list in a vertical ScrollView so the list (ActionsListView with actions:
store.actions, visibleCount: $visibleCount, onSelectAction: onSelectAction) can
scroll within the popover cap set by openPopover(), ensuring the “Load 10 more
actions…” button and lower rows remain reachable; place the ScrollView around
the ActionsListView (keeping bindings and callback intact) and constrain it to
the desired max height if needed so the popover still sizes correctly.
- Line 2: Remove the stale SwiftLint suppression by deleting the directive
"swiftlint:disable opening_brace" in PopoverMainView.swift (the commented
suppression line at the top of the file); ensure there are no other leftover
opening_brace disables in that region so CI sees no disabled rule left enabled
accidentally.
- Around line 421-449: RunnersListView currently filters runners by Runner.busy
and shows a yellow dot and MEM as a percent; change it to drive the subsection
from active local runners instead: update the activeRunners computed property to
filter for local, active runners (e.g. runner.isLocal && runner.isActive or the
equivalent local status field) rather than runner.busy, swap the indicator
Circle from Color.yellow to Color.green, and change the MEM display to show GB
(e.g. convert metrics.mem bytes to gigabytes and format like "MEM: %.1f GB")
while still showing CPU as percent; ensure you reference Runner,
RunnersListView, activeRunners, runner.metrics, and the Circle/Text lines when
making the edits.
- Around line 93-96: The onChange currently listens to store.actions (which
updates on metadata/poll refresh) and thus resets visibleCount too aggressively;
change the watcher to .onChange(of: store.actions.count) so the reset only
occurs when the number of actions changes, keeping the existing logic (if
visibleCount > 10 { visibleCount = 10 }) in the same block; update the observer
on the PopoverMainView where .onChange(of: store.actions) is used to reference
store.actions.count instead.
- Around line 179-188: The header "x" Button in PopoverMainView currently calls
NSApplication.shared.hide(nil) which only hides the app; change its action to
terminate the app instead by calling NSApplication.shared.terminate(nil) (or
NSApp.terminate(nil)) so the control performs the original "quit"
behavior—update the Button action in PopoverMainView (the Button with
Image(systemName: "xmark")) to call terminate(nil) rather than hide(nil).
- Around line 347-355: The view currently initializes the inline-job display cap
to 4 (State var cap = 4) but the spec/PR expects a 3-item default; change the
initial value to 3 and also make the "Show more" button increment match the same
chunk size (update the Button action from cap += 4 to cap += 3) so
ForEach(jobs.prefix(cap)) will start collapsed to 3 and expand in consistent
3-item steps; update any related comments or tests that assume the previous
4-item default.
In `@Sources/RunnerBar/RunnerStoreState.swift`:
- Around line 162-180: enrichGroupJobs currently uses cached steps but keeps the
live job's lifecycle fields, which can show in-progress even when cache is
terminal; update enrichGroupJobs so that when you choose cached.steps (i.e.,
when cached exists, cached.steps is non-empty, and you decide to use cache), you
also prefer cached lifecycle fields such as status and conclusion (and any
lifecycle timestamps already merged like startedAt/createdAt/completedAt) by
passing cached.status and cached.conclusion into the ActiveJob initializer
instead of job.status/job.conclusion; locate the enrichGroupJobs function and
the ActiveJob(...) return to make this change.
---
Nitpick comments:
In @.swiftlint.yml:
- Around line 42-44: Revert the global file_length relaxation in .swiftlint.yml
and restore the stricter warning threshold (keep the original lower warning
value) instead of setting warning: 500, and for any legitimately large
Sources/RunnerBar/*.swift files add a narrow, file-scoped suppression (e.g., add
a top-of-file // swiftlint:disable file_length and re-enable after the block) or
split the file into smaller units; target the file_length rule in .swiftlint.yml
and the specific Sources/RunnerBar/*.swift files when applying fixes.
In `@Sources/RunnerBar/ActionGroup.swift`:
- Around line 173-175: The custom Equatable implementation on ActionGroup (the
static func == in ActionGroup) only compares a subset of fields and should be
replaced by the synthesized Equatable; to do that, make WorkflowRunRef (the
element type of ActionGroup.runs) conform to Equatable (implement/derive
Equatable for WorkflowRunRef by adding conformance and ensuring its equality
covers status, conclusion, title, timestamps, repo, id, etc.), then remove the
hand-written static func == from ActionGroup so Swift can synthesize full-value
equality for ActionGroup.runs and all other stored properties.
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 100-452: The PopoverMainView file has grown too large—extract the
new subviews (MiniBarView, PopoverHeaderView including its statChip helper,
ActionsListView, ActionRowView and its helpers
actionStatusLabel/actionStatusColor/actionDotColor, InlineJobsView,
InlineJobRowView, and RunnersListView) into one or more small SwiftUI source
files so each type has a single responsibility; for each extracted struct create
a new file with import SwiftUI, preserve their current access level (private if
they are only used by PopoverMainView, otherwise internal), copy any small
helper methods used by that view (e.g., statChip, actionStatusLabel,
stepFraction, currentStepTitle) into the same file, update references in
PopoverMainView to the moved types, and run the build to fix any visibility or
module import issues.
🪄 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: 450267ed-ba3e-4fc8-a149-61a193e163ac
📒 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
+157
to
+169
| /// Completion fraction 0.0–1.0 for the pie-progress dot, or `nil` (indeterminate) | ||
| /// when status is queued or no job data is available. | ||
| /// | ||
| /// - `nil` → indeterminate (queued / no jobs loaded yet) | ||
| /// - `1.0` → completed | ||
| /// - `0..<1` → partial (jobsDone / jobsTotal) | ||
| var progressFraction: Double? { | ||
| switch groupStatus { | ||
| case .queued: return nil | ||
| 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.
Count all concluded jobs in the progress math.
progressFraction now rides on jobsDone, but jobsDone excludes failed and cancelled jobs. That makes an action with one failed job and one running job render as 0/2 and 0% progress even though half the jobs are already finished, so both the pie and the job-progress text underreport until the whole group completes.
Suggested fix
var progressFraction: Double? {
switch groupStatus {
case .queued: return nil
case .completed: return 1.0
case .inProgress:
- guard jobsTotal > 0 else { return nil }
- return Double(jobsDone) / Double(jobsTotal)
+ let completedJobs = jobs.filter { $0.conclusion != nil }.count
+ guard jobsTotal > 0 else { return nil }
+ return Double(completedJobs) / Double(jobsTotal)
}
}I’d drive jobProgress from the same “concluded jobs” count so the text and dot stay aligned.
📝 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
| /// Completion fraction 0.0–1.0 for the pie-progress dot, or `nil` (indeterminate) | |
| /// when status is queued or no job data is available. | |
| /// | |
| /// - `nil` → indeterminate (queued / no jobs loaded yet) | |
| /// - `1.0` → completed | |
| /// - `0..<1` → partial (jobsDone / jobsTotal) | |
| var progressFraction: Double? { | |
| switch groupStatus { | |
| case .queued: return nil | |
| case .completed: return 1.0 | |
| case .inProgress: | |
| guard jobsTotal > 0 else { return nil } | |
| return Double(jobsDone) / Double(jobsTotal) | |
| /// Completion fraction 0.0–1.0 for the pie-progress dot, or `nil` (indeterminate) | |
| /// when status is queued or no job data is available. | |
| /// | |
| /// - `nil` → indeterminate (queued / no jobs loaded yet) | |
| /// - `1.0` → completed | |
| /// - `0..<1` → partial (jobsDone / jobsTotal) | |
| var progressFraction: Double? { | |
| switch groupStatus { | |
| case .queued: return nil | |
| case .completed: return 1.0 | |
| case .inProgress: | |
| let completedJobs = jobs.filter { $0.conclusion != nil }.count | |
| guard jobsTotal > 0 else { return nil } | |
| return Double(completedJobs) / Double(jobsTotal) | |
| } | |
| } |
🤖 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 157 - 169, The
progressFraction currently uses jobsDone as the numerator which omits failed and
cancelled jobs; change it to use a "concluded" count (e.g. concluded = jobsDone
+ jobsFailed + jobsCancelled) so progressFraction returns
Double(concluded)/Double(jobsTotal) (guarding jobsTotal > 0), and update any
related consumer (such as jobProgress display logic) to use that same concluded
count so the pie and text remain consistent; locate and modify the
progressFraction computed property and the jobProgress rendering to reference
this concluded calculation (keep the .queued -> nil and .completed -> 1.0
branches unchanged).
Comment on lines
159
to
167
| let homeDir = FileManager.default.homeDirectoryForCurrentUser | ||
| .resolvingSymlinksInPath.path | ||
| .resolvingSymlinksInPath().path | ||
| let resolvedDir = URL(fileURLWithPath: dir) | ||
| .resolvingSymlinksInPath.path | ||
| .resolvingSymlinksInPath().path | ||
| guard resolvedDir.hasPrefix(homeDir) else { | ||
| DispatchQueue.main.async { | ||
| isRegistering = false | ||
| errorMessage = "Install directory must be inside your home folder (~/…)." | ||
| errorMessage = "Install directory must be inside your home folder (~/\u{2026})." | ||
| } |
There was a problem hiding this comment.
Harden home-directory boundary check for install path
resolvedDir.hasPrefix(homeDir) is path-boundary unsafe and can accept sibling paths (e.g. /Users/alice-evil/...). Validate with an exact match or homeDir + "/" boundary.
Suggested fix
- guard resolvedDir.hasPrefix(homeDir) else {
+ let isInsideHome = resolvedDir == homeDir || resolvedDir.hasPrefix(homeDir + "/")
+ guard isInsideHome else {
DispatchQueue.main.async {
isRegistering = false
errorMessage = "Install directory must be inside your home folder (~/\u{2026})."
}
return
}📝 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 homeDir = FileManager.default.homeDirectoryForCurrentUser | |
| .resolvingSymlinksInPath.path | |
| .resolvingSymlinksInPath().path | |
| let resolvedDir = URL(fileURLWithPath: dir) | |
| .resolvingSymlinksInPath.path | |
| .resolvingSymlinksInPath().path | |
| guard resolvedDir.hasPrefix(homeDir) else { | |
| DispatchQueue.main.async { | |
| isRegistering = false | |
| errorMessage = "Install directory must be inside your home folder (~/…)." | |
| errorMessage = "Install directory must be inside your home folder (~/\u{2026})." | |
| } | |
| let homeDir = FileManager.default.homeDirectoryForCurrentUser | |
| .resolvingSymlinksInPath().path | |
| let resolvedDir = URL(fileURLWithPath: dir) | |
| .resolvingSymlinksInPath().path | |
| let isInsideHome = resolvedDir == homeDir || resolvedDir.hasPrefix(homeDir + "/") | |
| guard isInsideHome else { | |
| DispatchQueue.main.async { | |
| isRegistering = false | |
| errorMessage = "Install directory must be inside your home folder (~/\u{2026})." | |
| } |
🤖 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 - 167, The current
boundary check using resolvedDir.hasPrefix(homeDir) is unsafe; update the guard
that compares homeDir and resolvedDir (computed from
FileManager.default.homeDirectoryForCurrentUser and URL(fileURLWithPath:
dir).resolvingSymlinksInPath().path) to ensure a true directory-boundary match —
e.g. normalize both to standardized/resolved URLs or paths and check that
resolvedDir equals homeDir or begins with homeDir + "/" (or compare
pathComponents to ensure homeDir's components are a prefix of resolvedDir's
components) before allowing the install path; adjust the guard and keep the
existing DispatchQueue.main.async error handling for the failing case.
Comment on lines
+279
to
+280
| let width = min(max(fittingWidth > 0 ? fittingWidth : Self.idealWidth, Self.idealWidth), Self.maxWidth) | ||
| let height = min(max(fittingHeight > 0 ? fittingHeight : 300, Self.minHeight), Self.maxHeight) |
There was a problem hiding this comment.
Fix the operator spacing in the new clamp expressions.
SwiftLint is failing on these lines, so this needs to be normalized before merge.
Suggested fix
- let width = min(max(fittingWidth > 0 ? fittingWidth : Self.idealWidth, Self.idealWidth), Self.maxWidth)
+ let width = min(max(fittingWidth > 0 ? fittingWidth : Self.idealWidth, Self.idealWidth), Self.maxWidth)
let height = min(max(fittingHeight > 0 ? fittingHeight : 300, Self.minHeight), Self.maxHeight)🧰 Tools
🪛 GitHub Check: SwiftLint
[failure] 279-279:
Operators should be surrounded by a single whitespace when they are being used (operator_usage_whitespace)
[failure] 279-279:
Operators should be surrounded by a single whitespace when they are being used (operator_usage_whitespace)
🤖 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 279 - 280, Normalize
operator spacing in the clamp expressions for width and height in AppDelegate
(variables: width, height) so SwiftLint passes: remove extra spaces and ensure
single spaces around binary operators and the ternary components (fittingWidth,
fittingHeight, the ">" comparison, the "?" and ":" parts, and the commas
separating arguments) when referencing Self.idealWidth, Self.minHeight,
Self.maxWidth, etc.; update the expressions that use max(...), min(...) and the
ternary checks (fittingWidth > 0 ? fittingWidth : Self.idealWidth and
fittingHeight > 0 ? fittingHeight : 300) to have consistent single-space
operator spacing.
| @@ -1,66 +1,66 @@ | |||
| import SwiftUI | |||
| // swiftlint:disable opening_brace | |||
There was a problem hiding this comment.
Remove the stale SwiftLint suppression.
opening_brace is already not firing inside this region, so the disable itself is now the lint failure blocking CI.
🧰 Tools
🪛 GitHub Check: SwiftLint
[failure] 2-2:
SwiftLint rule 'opening_brace' 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/PopoverMainView.swift` at line 2, Remove the stale
SwiftLint suppression by deleting the directive "swiftlint:disable
opening_brace" in PopoverMainView.swift (the commented suppression line at the
top of the file); ensure there are no other leftover opening_brace disables in
that region so CI sees no disabled rule left enabled accidentally.
Comment on lines
+77
to
+83
| // ⚠️ NO ScrollView — let the VStack grow naturally so fittingSize captures | ||
| // all inline job rows. AppDelegate caps height at 620pt. | ||
| ActionsListView( | ||
| actions: store.actions, | ||
| visibleCount: $visibleCount, | ||
| onSelectAction: onSelectAction | ||
| ) |
There was a problem hiding this comment.
Restore a scroll container around the paged actions list.
openPopover() hard-caps the popover height to 620 pt, so without an internal ScrollView anything below that cutoff is just clipped. Once visibleCount grows, lower action rows, inline jobs, and eventually the “Load 10 more actions…” button can become unreachable, which breaks the #305 scrolling requirement.
🤖 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 77 - 83, The
ActionsListView is being added directly to the VStack which causes content to be
clipped by the popover height cap; wrap the paged actions list in a vertical
ScrollView so the list (ActionsListView with actions: store.actions,
visibleCount: $visibleCount, onSelectAction: onSelectAction) can scroll within
the popover cap set by openPopover(), ensuring the “Load 10 more actions…”
button and lower rows remain reachable; place the ScrollView around the
ActionsListView (keeping bindings and callback intact) and constrain it to the
desired max height if needed so the popover still sizes correctly.
Comment on lines
+93
to
+96
| // ⚠️ Only reset when user has paged past default — do NOT reset on every poll tick. | ||
| .onChange(of: store.actions) { _ in | ||
| if visibleCount > 10 { visibleCount = 10 } | ||
| } |
There was a problem hiding this comment.
Reset pagination on count changes only.
.onChange(of: store.actions) still fires on normal poll refreshes when action metadata changes, so anyone who paged past 10 gets snapped back to the default list on the next update. The reset needs to track store.actions.count, not the whole array.
Suggested fix
- .onChange(of: store.actions) { _ in
+ .onChange(of: store.actions.count) { _ in
if visibleCount > 10 { visibleCount = 10 }
}🤖 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 93 - 96, The onChange
currently listens to store.actions (which updates on metadata/poll refresh) and
thus resets visibleCount too aggressively; change the watcher to .onChange(of:
store.actions.count) so the reset only occurs when the number of actions
changes, keeping the existing logic (if visibleCount > 10 { visibleCount = 10 })
in the same block; update the observer on the PopoverMainView where
.onChange(of: store.actions) is used to reference store.actions.count instead.
Comment on lines
+179
to
+188
| 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.
Make the header x actually quit.
The redesign moved the quit affordance into the header, but this action only hides the app. That leaves the old “quit” behavior gone from the UI even though the control is rendered as the replacement.
Suggested fix
Button(
- action: { NSApplication.shared.hide(nil) },
+ action: { NSApplication.shared.terminate(nil) },
label: {
Image(systemName: "xmark")
.font(.system(size: 11))
.foregroundColor(.secondary)
}
)
.buttonStyle(.plain)
- .help("Close popover")
+ .help("Quit")📝 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
| Button( | |
| action: { NSApplication.shared.hide(nil) }, | |
| label: { | |
| Image(systemName: "xmark") | |
| .font(.system(size: 11)) | |
| .foregroundColor(.secondary) | |
| } | |
| ) | |
| .buttonStyle(.plain) | |
| .help("Close popover") | |
| Button( | |
| action: { NSApplication.shared.terminate(nil) }, | |
| label: { | |
| Image(systemName: "xmark") | |
| .font(.system(size: 11)) | |
| .foregroundColor(.secondary) | |
| } | |
| ) | |
| .buttonStyle(.plain) | |
| .help("Quit") |
🤖 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 179 - 188, The header
"x" Button in PopoverMainView currently calls NSApplication.shared.hide(nil)
which only hides the app; change its action to terminate the app instead by
calling NSApplication.shared.terminate(nil) (or NSApp.terminate(nil)) so the
control performs the original "quit" behavior—update the Button action in
PopoverMainView (the Button with Image(systemName: "xmark")) to call
terminate(nil) rather than hide(nil).
Comment on lines
+347
to
+355
| @State private var cap: Int = 4 | ||
|
|
||
| var body: some View { | ||
| ForEach(jobs.prefix(cap)) { job in | ||
| InlineJobRowView(job: job) | ||
| } | ||
| if jobs.count > cap { | ||
| Button( | ||
| action: { cap += 4 }, |
There was a problem hiding this comment.
The inline-job default cap is off by one.
The PR notes call out a 3-inline-job display cap in the view layer, but this starts expanded at 4.
Suggested fix
- `@State` private var cap: Int = 4
+ `@State` private var cap: Int = 3🤖 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 347 - 355, The view
currently initializes the inline-job display cap to 4 (State var cap = 4) but
the spec/PR expects a 3-item default; change the initial value to 3 and also
make the "Show more" button increment match the same chunk size (update the
Button action from cap += 4 to cap += 3) so ForEach(jobs.prefix(cap)) will start
collapsed to 3 and expand in consistent 3-item steps; update any related
comments or tests that assume the previous 4-item default.
Comment on lines
+421
to
+449
| /// Conditional runners sub-section — only shown when ≥1 Runner is busy (Phase 6 / #307). | ||
| private struct RunnersListView: View { | ||
| let runners: [Runner] | ||
|
|
||
| private var activeRunners: [Runner] { runners.filter { $0.busy } } | ||
|
|
||
| /// Opens the GitHub PAT setup docs in the default browser. | ||
| /// NSAppleScript/Terminal removed — device-flow requires a user_code the app never generates. | ||
| /// Auth.swift resolves token via: gh auth token → GH_TOKEN → GITHUB_TOKEN (ref #221 #246). | ||
| private func signInWithGitHub() { | ||
| let urlString = "https://docs.github.com/en/authentication/" + | ||
| "keeping-your-account-and-data-secure/managing-your-personal-access-tokens" | ||
| guard let url = URL(string: urlString) else { return } | ||
| NSWorkspace.shared.open(url) | ||
| var body: some View { | ||
| if !activeRunners.isEmpty { | ||
| ForEach(activeRunners, id: \.id) { runner in | ||
| 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: — MEM: —").font(.caption).foregroundColor(.secondary) | ||
| } | ||
| Image(systemName: "chevron.right") | ||
| .font(.caption2).foregroundColor(.secondary.opacity(0.4)) | ||
| } | ||
| .padding(.horizontal, 12).padding(.vertical, 3) | ||
| } | ||
| Divider() |
There was a problem hiding this comment.
Drive this section from active local runners, not busy API runners.
#307 calls for a subsection that appears when local runners are active and shows a green dot plus CPU% and MEM in GB. This implementation filters Runner.busy, renders a yellow dot, and formats memory as a percentage, so it can show the wrong rows and the wrong metrics.
🤖 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 421 - 449,
RunnersListView currently filters runners by Runner.busy and shows a yellow dot
and MEM as a percent; change it to drive the subsection from active local
runners instead: update the activeRunners computed property to filter for local,
active runners (e.g. runner.isLocal && runner.isActive or the equivalent local
status field) rather than runner.busy, swap the indicator Circle from
Color.yellow to Color.green, and change the MEM display to show GB (e.g. convert
metrics.mem bytes to gigabytes and format like "MEM: %.1f GB") while still
showing CPU as percent; ensure you reference Runner, RunnersListView,
activeRunners, runner.metrics, and the Circle/Text lines when making the edits.
Comment on lines
+162
to
+180
| /// Merges richer step data from `jobCache` into group jobs where available. | ||
| func enrichGroupJobs(_ jobs: [ActiveJob], jobCache: [Int: ActiveJob]) -> [ActiveJob] { | ||
| jobs.map { job in | ||
| guard let cached = jobCache[job.id], | ||
| !cached.steps.isEmpty, | ||
| job.steps.isEmpty | ||
| || job.steps.contains(where: { $0.status == "in_progress" }) | ||
| else { return job } | ||
| 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.
Use the cached lifecycle fields when the cache wins.
enrichGroupJobs currently grafts in cached steps/timestamps but keeps the older live status and conclusion. If jobCache has already observed a completion while fetchActionGroups still returns stale live data, the merged row can keep rendering as in-progress even though the backfilled steps are terminal.
Suggested fix
return ActiveJob(
id: job.id, name: job.name, status: job.status,
- conclusion: job.conclusion,
+ status: cached.status,
+ conclusion: cached.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 162 - 180,
enrichGroupJobs currently uses cached steps but keeps the live job's lifecycle
fields, which can show in-progress even when cache is terminal; update
enrichGroupJobs so that when you choose cached.steps (i.e., when cached exists,
cached.steps is non-empty, and you decide to use cache), you also prefer cached
lifecycle fields such as status and conclusion (and any lifecycle timestamps
already merged like startedAt/createdAt/completedAt) by passing cached.status
and cached.conclusion into the ActiveJob initializer instead of
job.status/job.conclusion; locate the enrichGroupJobs function and the
ActiveJob(...) return to make this change.
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.
User description
Summary
Implements the full
PopoverMainViewredesign as specified in #296, #178 (comment) and #178 (comment).Phases
PieProgressViewcomponent (closes Phase 1 — New PieProgressView component #297)Closes
Closes #296
Closes #297
Closes #299
Closes #302
Closes #304
Closes #305
Closes #307
Regression Guards
.frame(idealWidth: 420, maxWidth: .infinity, alignment: .top)on rootVStackpreserved (ref ⚠️ Regression guard: rules for editing PopoverMainView, AppDelegate & ActiveJob #52, 🚨 DO NOT REPEAT: Definitive regression anti-pattern guide (read before every edit) #54, 🔴 DEFINITIVE REGRESSION MAP — read every single time before editing AppDelegate or PopoverMainView #57).lineLimit(1)insideSystemStatsViewpreserved (load-bearing for fittingSize).padding(.horizontal, 12)Notes
visibleCountresets to 10 on each store poll via.onChange(of: store.actions.count)RunnerStoreObservableCodeAnt-AI Description
Redesign the popover to show live system stats, runners, and actions in a single scrolling view
What Changed
Impact
✅ Clearer workflow status at a glance✅ Fewer popover jumps when opening details✅ Easier runner setup and live runner monitoring🔄 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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores