feat: PopoverMainView redesign (#296)#311
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable SwiftUI ChangesUI Components and State Bridge for PopoverMainView Redesign Phase 1
Sequence Diagram(s)sequenceDiagram
participant UI as PopoverMainView
participant Stats as SystemStatsView
participant Obs as RunnerStoreObservable
participant Store as RunnerStore.shared
UI->>Obs: subscribes to `@Published` actions/runners/jobs
Obs->>Store: read current state (reload)
Store-->>Obs: returns actions/jobs/runners/isRateLimited
Obs-->>UI: publishes updates
UI->>Stats: embed statsContent for header
UI->>UI: render actions list (PieProgressView, inline ↳ jobs, runners)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
Review Summary by QodoAdd PieProgressView and RunnerStoreObservable for redesign
WalkthroughsDescription• Add PieProgressView component for circular progress indicators • Implement RunnerStoreObservable bridge for SwiftUI state management • Enable live runner/job/action data binding to views Diagramflowchart LR
RS["RunnerStore.shared"]
RSO["RunnerStoreObservable"]
PV["PieProgressView"]
Views["SwiftUI Views"]
RS -- "reload()" --> RSO
RSO -- "@Published properties" --> Views
PV -- "progress indicator" --> Views
File Changes1. Sources/RunnerBar/PieProgressView.swift
|
Code Review by Qodo
1. Pie dot has halo
|
🐙 Octopus Review — PR #296SummaryThis PR is a significant redesign of Score
Risk Assessment
Positive Highlights
Important Files Changed
Diagramgraph TD
AppDelegate["AppDelegate"]
PopoverMainView["PopoverMainView\n(redesigned)"]
PopoverHeaderView["PopoverHeaderView\n(new)"]
RunnersListView["RunnersListView\n(new — busy only)"]
ActionsListView["ActionsListView\n(new + pagination)"]
ActionRowView["ActionRowView\n(new)"]
InlineJobsView["InlineJobsView\n(new)"]
InlineJobRowView["InlineJobRowView\n(new)"]
PieProgressView["PieProgressView\n(new)"]
RelativeTimeFormatter["RelativeTimeFormatter\n(new)"]
ActionRunsResponse["ActionRunsResponse.swift\n(extracted)"]
RunnerStoreObservable["RunnerStoreObservable\n(@MainActor — extracted)"]
SystemStatsViewModel["SystemStatsViewModel\n(start/stop lifecycle)"]
Navigate["navigate()\n(now resizes popover)"]
AppDelegate --> PopoverMainView
AppDelegate --> Navigate
PopoverMainView --> PopoverHeaderView
PopoverMainView --> RunnersListView
PopoverMainView --> ActionsListView
PopoverMainView --> RunnerStoreObservable
ActionsListView --> ActionRowView
ActionRowView --> InlineJobsView
ActionRowView --> PieProgressView
InlineJobsView --> InlineJobRowView
InlineJobRowView --> PieProgressView
PopoverHeaderView --> SystemStatsViewModel
ActionRowView --> RelativeTimeFormatter
ActionRunsResponse -. "extracted from" .-> ActionGroup["ActionGroup.swift"]
classDef changed fill:#fbbf24,stroke:#92400e,color:#000
classDef added fill:#4ade80,stroke:#166534,color:#000
classDef unchanged fill:#374151,stroke:#6b7280,color:#e5e7eb
class PopoverMainView,AppDelegate,RunnerStoreObservable,SystemStatsViewModel,ActionGroup changed
class PopoverHeaderView,RunnersListView,ActionsListView,ActionRowView,InlineJobsView,InlineJobRowView,PieProgressView,RelativeTimeFormatter,ActionRunsResponse added
class Navigate unchanged
Last reviewed commit: local Checklist
Reviewed by Octopus |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// All known self-hosted runners. | ||
| @Published private(set) var runners: [Runner] = [] | ||
| /// `true` when the most recent poll hit a GitHub rate limit. | ||
| @Published private(set) var isRateLimited: Bool = false |
There was a problem hiding this comment.
🟡 private(set) on @Published property has no effect — mutations inside the module are unrestricted
private(set) restricts the synthesized setter to the declaring file, but @Published wraps the stored property in a Published<T> struct whose projected value ($actions, etc.) remains accessible anywhere in the module. More importantly, the _actions backing storage can still be mutated from other files within the same module via @Published's internal mechanics. In practice the intent is correct — only reload() should write — but the access-control annotation does not enforce this. Consider private var _actions: [ActionGroup] = [] with a computed var actions: [ActionGroup] { _actions } pattern, or accept the current state and add a comment that private(set) is documentation-by-convention rather than enforcement.
| @Published private(set) var isRateLimited: Bool = false | |
| // Option A — document the intent explicitly: | |
| /// External reads only. Write access intentionally limited to reload(). | |
| @Published private(set) var actions: [ActionGroup] = [] | |
| // Option B — if strict enforcement is needed, remove @Published and | |
| // drive change notifications manually: | |
| private var _actions: [ActionGroup] = [] { | |
| willSet { objectWillChange.send() } | |
| } | |
| var actions: [ActionGroup] { _actions } |
AI Fix Prompt
Fix the following Medium (Bug) issue in `Sources/RunnerBar/RunnerStoreObservable.swift` at lines 12-19:
Problem: `private(set)` restricts the synthesized setter to the declaring file, but `@Published` wraps the stored property in a `Published<T>` struct whose projected value (`$actions`, etc.) remains accessible anywhere in the module. More importantly, the `_actions` backing storage can still be mutated from other files within the same module via `@Published`'s internal mechanics. In practice the intent is correct — only `reload()` should write — but the access-control annotation does not enforce this. Consider `private var _actions: [ActionGroup] = []` with a computed `var actions: [ActionGroup] { _actions }` pattern, or accept the current state and add a comment that `private(set)` is documentation-by-convention rather than enforcement.
Suggested fix:
// Option A — document the intent explicitly:
/// External reads only. Write access intentionally limited to reload().
@Published private(set) var actions: [ActionGroup] = []
// Option B — if strict enforcement is needed, remove @Published and
// drive change notifications manually:
private var _actions: [ActionGroup] = [] {
willSet { objectWillChange.send() }
}
var actions: [ActionGroup] { _actions }
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| Circle() | ||
| .trim(from: 0, to: CGFloat(progress)) | ||
| .rotation(.degrees(-90)) | ||
| .stroke(color, style: StrokeStyle(lineWidth: size * 0.25, lineCap: .round)) |
There was a problem hiding this comment.
Suggestion: The partial-progress branch draws only a stroked arc on the circumference, which does not match the component's stated "radial pie fill" behavior for 0 < progress < 1. This will render a ring segment instead of a filled pie wedge, so intermediate progress states are visually incorrect. Render a filled sector (pie slice) for partial progress instead of using stroke on a trimmed circle. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ PieProgressView shows ring arc not radial pie.
- ⚠️ In-progress jobs miscommunicate completion fraction visually.Steps of Reproduction ✅
1. In any SwiftUI view, render `PieProgressView(progress: 0.5, color: .green)` so that the
view defined in `Sources/RunnerBar/PieProgressView.swift:13-35` is displayed on screen.
2. At runtime, the `body` computed property at
`Sources/RunnerBar/PieProgressView.swift:21-35` evaluates the conditional; with `0.0 <
progress < 1.0`, the `else if progress > 0` branch at line 27 executes.
3. That branch constructs a `Circle()` trimmed from 0 to `CGFloat(progress)` and rotated
(lines 28-30), then applies `.stroke(color, style: StrokeStyle(lineWidth: size * 0.25,
lineCap: .round))` at line 31, which draws only the circumference as an arc segment.
4. On screen, you observe a thin ring arc over the faint outline (lines 23-24) rather than
a filled radial pie wedge, which contradicts the documented "radial pie fill" behavior
described in the header comment at `Sources/RunnerBar/PieProgressView.swift:5-10`.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/PieProgressView.swift
**Line:** 28:31
**Comment:**
*Incomplete Implementation: The partial-progress branch draws only a stroked arc on the circumference, which does not match the component's stated "radial pie fill" behavior for `0 < progress < 1`. This will render a ring segment instead of a filled pie wedge, so intermediate progress states are visually incorrect. Render a filled sector (pie slice) for partial progress instead of using `stroke` on a trimmed circle.
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: 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/RunnerStoreObservable.swift`:
- Around line 11-34: The RunnerStoreObservable class mutates several `@Published`
properties from reload() without compile-time main-thread enforcement; annotate
the type with `@MainActor` (i.e., add `@MainActor` before final class
RunnerStoreObservable) so all methods including reload() and property accesses
are isolated to the main actor, preventing background calls from compiling and
matching the pattern used by LocalRunnerStore.refresh().
🪄 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: 5e7313cb-07ac-4456-a746-62ba8eb4953f
📒 Files selected for processing (2)
Sources/RunnerBar/PieProgressView.swiftSources/RunnerBar/RunnerStoreObservable.swift
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 39-58: The icon-only buttons in PopoverMainView.swift lack
accessibility labels and the 7x7 auth Circle is too small; update the Button
that calls signInWithGitHub to add an explicit accessibilityLabel (e.g., "Sign
in with GitHub") and increase its hit area by wrapping the Circle in a larger
tappable container (e.g., apply padding or a fixed frame/ContentShape so the
touch target is at least ~44x44 points) while keeping the visible circle small,
and add explicit accessibilityLabel modifiers to the settings Button
(onSelectSettings, e.g., "Settings") and the quit Button
(NSApplication.shared.terminate, e.g., "Quit RunnerBar") so each control has a
clear accessible name.
🪄 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: faf9052d-f8cc-480a-99bc-118e39d0a3e5
📒 Files selected for processing (2)
Sources/RunnerBar/PopoverMainView.swiftSources/RunnerBar/SystemStatsView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/RunnerBar/PopoverMainView.swift (1)
41-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit accessibility names and a real hit target for the header controls.
The auth badge is still a 7x7 tap target, and the three icon-only controls still rely on visual affordances/tooltips instead of explicit accessibility labels. That keeps the VoiceOver issue from the earlier review unresolved.
🤖 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 41 - 61, The three header controls (auth badge, settings, quit) currently have tiny or no real hit targets and lack explicit VoiceOver labels; update the Button for signInWithGitHub (the Circle badge) and the Buttons that call onSelectSettings and NSApplication.shared.terminate to provide explicit accessibility labels via .accessibilityLabel(...) (e.g., "Sign in with GitHub", "Settings", "Quit RunnerBar") and expand their hit areas (e.g., add padding or a minimum tappable frame like minWidth/minHeight or .contentShape(Rectangle()) around the Circle/Image) so the small Circle().fill(...) used in signInWithGitHub becomes a visually small badge but a larger tappable target; ensure these changes are applied to the Button wrappers around signInWithGitHub, onSelectSettings and the quit action.
🧹 Nitpick comments (1)
Sources/RunnerBar/PopoverMainView.swift (1)
86-243: 🏗️ Heavy liftPlease extract the action-list row rendering into dedicated views/files.
PopoverMainViewis now carrying header chrome, auth affordances, pagination state, action-row layout, inline-job-row layout, and status-formatting helpers in one type. This redesign would be much easier to maintain if the row views moved out instead of growing this file further.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 86 - 243, PopoverMainView is doing too much: extract the action-row and inline-job-row UI and related small helpers into dedicated SwiftUI view structs/files (e.g. ActionRowView and InlineJobRowView) to reduce file size. Move the ForEach body that renders a Button for each actionGroup (the HStack using PieProgressView, Text(actionGroup.label), Text(actionGroup.title), startedAgo/elapsed/jobProgress/status and chevron) into ActionRowView that accepts an ActionGroup, onSelectAction callback, and styling helpers (actionDotColor, actionStatusLabel, actionStatusColor) via parameters or a small protocol/closure set; similarly move the inner ForEach job Button HStack into InlineJobRowView accepting ActiveJob and onSelectJob plus jobDotColor. Keep pure formatting helpers (actionStatusLabel/actionStatusColor/actionDot/jobDot) either as static helpers in a small new file or pass them in, and update PopoverMainView to use these new views while preserving visibleCount pagination and Load more button behavior.
🤖 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 31-32: PopoverMainView's pagination is sticky because visibleCount
is never reset and the "Load more" button doesn't show a terminal "No more
actions" state; update the Load more UI to show the button only when
visibleCount < actions.count and otherwise show a disabled/terminal "No more
actions" view, and reset visibleCount back to its initial value (10) whenever
the underlying actions list or scope changes (e.g., add an onChange/onReceive
for the actions/scope that sets visibleCount = 10). Use the existing
visibleCount state and the action list identifier(s) in PopoverMainView to
implement the conditional rendering and the reset.
- Around line 138-173: The inline job rows are currently interactive but should
be passive informational rows: remove the Button wrapper, the onSelectJob call,
the .buttonStyle(.plain) and the chevron.right Image; instead render a static
HStack per job (inside the same ForEach over actionGroup.jobs) that uses
PieProgressView with a computed fraction from the job's steps (currentStepIndex
/ max(1, totalSteps)) instead of the hard-coded 0.5, and replace the job
name/status texts with the active step title and a step fraction label (e.g.,
"3/5" or "Step: Running tests"); keep jobDotColor(for: job), job.elapsed
display, padding, and accessibility but ensure the row is not tappable or
navigable.
---
Duplicate comments:
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 41-61: The three header controls (auth badge, settings, quit)
currently have tiny or no real hit targets and lack explicit VoiceOver labels;
update the Button for signInWithGitHub (the Circle badge) and the Buttons that
call onSelectSettings and NSApplication.shared.terminate to provide explicit
accessibility labels via .accessibilityLabel(...) (e.g., "Sign in with GitHub",
"Settings", "Quit RunnerBar") and expand their hit areas (e.g., add padding or a
minimum tappable frame like minWidth/minHeight or .contentShape(Rectangle())
around the Circle/Image) so the small Circle().fill(...) used in
signInWithGitHub becomes a visually small badge but a larger tappable target;
ensure these changes are applied to the Button wrappers around signInWithGitHub,
onSelectSettings and the quit action.
---
Nitpick comments:
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 86-243: PopoverMainView is doing too much: extract the action-row
and inline-job-row UI and related small helpers into dedicated SwiftUI view
structs/files (e.g. ActionRowView and InlineJobRowView) to reduce file size.
Move the ForEach body that renders a Button for each actionGroup (the HStack
using PieProgressView, Text(actionGroup.label), Text(actionGroup.title),
startedAgo/elapsed/jobProgress/status and chevron) into ActionRowView that
accepts an ActionGroup, onSelectAction callback, and styling helpers
(actionDotColor, actionStatusLabel, actionStatusColor) via parameters or a small
protocol/closure set; similarly move the inner ForEach job Button HStack into
InlineJobRowView accepting ActiveJob and onSelectJob plus jobDotColor. Keep pure
formatting helpers (actionStatusLabel/actionStatusColor/actionDot/jobDot) either
as static helpers in a small new file or pass them in, and update
PopoverMainView to use these new views while preserving visibleCount pagination
and Load more button behavior.
🪄 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: d4e7264c-3725-4d4f-a6d7-d2144d326c64
📒 Files selected for processing (2)
Sources/RunnerBar/ActionGroup.swiftSources/RunnerBar/PopoverMainView.swift
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/RunnerBar/PopoverMainView.swift (1)
88-217: 🏗️ Heavy liftPlease extract the new row/layout logic out of
PopoverMainView.This view now owns pagination, action rows, inline job rows, runner rows, and multiple status-mapping helpers. Splitting those into dedicated
Sources/RunnerBar/*.swiftviews would bring this file back toward single-responsibility and make the redesign easier to maintain.As per coding guidelines,
Sources/RunnerBar/*.swift:Keep files small and single-responsibility — add new files rather than growing existing ones.Also applies to: 247-293
🤖 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 88 - 217, PopoverMainView has grown too large: extract the action row, inline job row, runners list, and pagination into small view files under Sources/RunnerBar/*.swift. Create ActionRowView (renders a single actionGroup row and uses PieProgressView, actionStatusLabel/actionStatusColor/actionDotColor helpers), JobRowView (renders a single job row and uses jobDotColor), ActionListView (wraps the ScrollView + LazyVStack, handles visibleCount pagination and the "Load more" button, calls onSelectAction/onSelectJob), and RunnerRowView (renders each runner row); move status-mapping helpers (actionStatusLabel, actionStatusColor, actionDotColor, jobDotColor) into a shared helper or ViewModel used by these new views, then replace the large inline blocks in PopoverMainView with calls to ActionListView and a ForEach of RunnerRowView, wiring up the same parameters and callbacks.
🤖 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 43-49: The popover currently uses signInWithGitHub and a help
string that instructs users to "set up a GitHub token", which violates the rule
against prompting for PATs inside the app; change the Button's action from
signInWithGitHub to a neutral settings/auth entry point (e.g., openAuthSettings
or openPreferences) and update the help text to a neutral message such as "Not
authenticated — open Settings to connect an account" (remove any mention of
token/PAT), leaving the visual Circle/button style unchanged so callers
referencing isAuthenticated, Button, and the help modifier still work.
- Around line 195-217: The runner subsection is currently rendered after the
actions list and only shows statusDescription; move the active runners block
(the logic using localRunners.runners and the activeRunners filter) so it
appears between the header and the actions list, and for each runner (identified
by runner in the ForEach) replace the current small HStack (which uses
runnerDotColor(for:), runner.runnerName, runner.statusDescription) with a
chevron-style row that shows the runner dot, runner.runnerName, and on the
trailing side CPU% and memory in GB (calculate and format per-runner values from
the Runner model), keeping truncation/lineLimit behavior and using
runnerDotColor(for:) for both dot and the right-side text color; ensure layout
matches the chevron affordance (leading content, trailing stacked CPU% and MEM
(GB), and a chevron indicator).
- Around line 57-63: The Button in PopoverMainView currently calls
NSApplication.shared.terminate(nil) which quits the app; change the
Button(action:) to dismiss the popover instead. Replace the terminate call
inside the Button action with logic that closes the popover hosting this view
(for example use presentationMode.wrappedValue.dismiss() if the view has
`@Environment`(\\.presentationMode) var presentationMode, or call
NSPopover.performClose(sender) / close the hosting window) so the "xmark" only
closes the popover; update any required `@Environment` or popover reference in
PopoverMainView to enable that dismissal.
- Around line 247-261: The actionStatusLabel(for:) function currently returns
mixed-case labels; update it to return the approved Phase 3 uppercase labels. In
actionStatusLabel(for:) map group.groupStatus .inProgress -> "IN PROGRESS",
.queued -> "QUEUED", and for .completed map group.conclusion values to
uppercase: "success" -> "SUCCESS", "failure" -> "FAILED", both "cancelled" and
"canceled" -> "CANCELED", "skipped" -> "SKIPPED", and use "DONE" as the default;
keep the function and switch structure (actionStatusLabel, group.groupStatus,
group.conclusion) unchanged.
---
Nitpick comments:
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 88-217: PopoverMainView has grown too large: extract the action
row, inline job row, runners list, and pagination into small view files under
Sources/RunnerBar/*.swift. Create ActionRowView (renders a single actionGroup
row and uses PieProgressView, actionStatusLabel/actionStatusColor/actionDotColor
helpers), JobRowView (renders a single job row and uses jobDotColor),
ActionListView (wraps the ScrollView + LazyVStack, handles visibleCount
pagination and the "Load more" button, calls onSelectAction/onSelectJob), and
RunnerRowView (renders each runner row); move status-mapping helpers
(actionStatusLabel, actionStatusColor, actionDotColor, jobDotColor) into a
shared helper or ViewModel used by these new views, then replace the large
inline blocks in PopoverMainView with calls to ActionListView and a ForEach of
RunnerRowView, wiring up the same parameters and callbacks.
🪄 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: d6a83ee0-d2bc-4ff9-9d95-0ac18274b011
📒 Files selected for processing (1)
Sources/RunnerBar/PopoverMainView.swift
Code Review — PR #311
|
| # | Severity | File | Fix needed |
|---|---|---|---|
| 1 | 🔴 Bug | PopoverMainView.swift |
actionStatusLabel → uppercase per spec |
| 2 | 🔴 Bug | PopoverMainView.swift |
Child row status text → uppercase |
| 3 | 🔴 Bug | PopoverMainView.swift |
[×] → hide not terminate |
| 4 | 🔴 Bug | PopoverMainView.swift |
Child PieProgressView progress → real step fraction |
| 5 | 🟡 Polish | PopoverMainView.swift |
Fix ScrollView/LazyVStack indent |
| 6 | 🟡 Polish | PopoverMainView.swift |
Simplify Load more label to static string |
| 7 | 🟡 Spec | PopoverMainView.swift |
Remove Text("Actions") section label |
| 8 | 🟡 Spec | PopoverMainView.swift |
Runner rows → Button + > chevron |
| 9 | 🟡 Spec | PopoverMainView.swift |
Remove Text("Runners") section label |
| 10 | 🟡 Doc | RunnerStoreObservable.swift |
Fix comment: "capped at 5" → "capped at 10" |
| 11 | 🟡 Lifecycle | PopoverMainView.swift |
Add .onDisappear { systemStats.stop() } |
Items 1–4 should be resolved before merge. Items 5–11 are polish/spec conformance that would be good to land in the same PR rather than follow-ups.
🔴 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"
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:L
|
CodeAnt AI Incremental review completed. |
- #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
🔍 Code Review — PR #311:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
Sources/RunnerBar/PopoverMainView.swift (5)
42-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the unauthenticated affordance neutral.
The help text still tells users to add a GitHub token. This flow was already redirected to Settings, so the copy should stay at that neutral level too.
As per coding guidelines,
Sources/RunnerBar/*.swift:Do not prompt the user for a GitHub token or PAT directly in the app.🤖 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 42 - 49, The help text currently prompts the user to "add a GitHub token" even though the button now routes to Settings; update the user-facing copy to be neutral by changing the .help text on the unauthenticated Button (the Button using onSelectSettings and the Circle indicator in PopoverMainView/PopoverMainView.swift) to something like "Not authenticated — open Settings" so it does not mention adding a GitHub token or PAT.
43-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit accessibility names to the header controls.
The auth dot is still a tiny unlabeled target, and the gear / x buttons are icon-only without clear accessible names. That keeps the header hard to use with VoiceOver and makes the unauthenticated affordance easy to miss.
🤖 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 43 - 64, The header buttons lack accessibility labels and the auth dot is a tiny unlabeled target; update the Button for the auth dot (the branch guarded by isAuthenticated and the Circle() view) to have a clear accessibility label (e.g., "Not authenticated, open Settings") and a larger hit area if needed, and add explicit .accessibilityLabel(...) to the Settings button (Image(systemName: "gearshape") / onSelectSettings) and the Hide button (Image(systemName: "xmark") / NSApplication.shared.hide(nil)) so VoiceOver announces their purpose; ensure labels are concise and consider adding .accessibilityAddTraits(.isButton) or a descriptive accessibilityHint where helpful.
149-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRender step-level details in the inline job rows.
These rows still show
job.nameand a job-status label, and they still fall back to a synthetic0.5whenjob.stepsis empty. Phase 4 calls for the current step title plus adone/totalfraction, so the child rows still aren't surfacing the step data the redesign was meant to expose.🤖 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 149 - 175, The inline job rows currently display job.name and a synthetic 0.5 progress when job.steps is empty; instead compute and surface the current step title and a done/total fraction and remove the 0.5 fallback: update the stepProgress closure (job.steps) to compute total = job.steps.count, done = job.steps.filter { $0.conclusion != nil }.count, progress = total > 0 ? Double(done)/Double(total) : 0.0, and derive currentStepTitle = job.steps.first(where: { $0.conclusion == nil })?.title ?? job.steps.first?.title ?? "No steps"; replace Text(job.name) with Text(currentStepTitle) and replace the status/label Text that currently shows "IN PROGRESS"/"QUEUED" or the static fraction with a Text showing "\(done)/\(total)" (or "0/0" when empty) while keeping PieProgressView(progress: stepProgress, ... ) and existing color/size/formatting.
197-223:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPhase 6 is still incomplete in the runners block.
This subsection is still rendered after the actions list and the rows still show
statusDescription; the spec calls for the runners block between the header and actions, with per-runner CPU% and MEM (GB) in the trailing slot.🤖 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 197 - 223, The runners block is in the wrong place and displaying the wrong trailing info; move the activeRunners block (the ForEach over localRunners.runners) so it appears between the header and the actions list, remove any section label, and replace the trailing Text(runner.statusDescription) with formatted CPU% and MEM (GB) values (use the model properties like runner.cpuUsagePercent and runner.memoryGB or compute them from existing fields) while keeping runnerDotColor(for:), runner.runnerName and the chevron; ensure lineLimit/truncation and padding stay the same and retain .buttonStyle(.plain).
31-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
visibleCountwhen the actions dataset changes.After a user expands the list once, later refreshes still inherit that larger count, so the Phase 5 default of 10 never comes back and the terminal “No more actions” state still can't be shown.
Also applies to: 181-190, 227-235
🤖 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 31 - 32, The visibleCount state must be reset to the default when the underlying actions dataset changes; add logic in PopoverMainView to set visibleCount = 10 whenever the actions collection/property updates (e.g. use SwiftUI's .onChange(of: actions) { _ in visibleCount = 10 } or reset in the actions property's didSet / the view model observer). Apply the same reset behavior where the other action lists are handled (the blocks around the code referenced by symbols at the other ranges) so any refresh/replace of the actions array restores the Phase 5 default and allows "No more actions" to reappear.
🤖 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/PieProgressView.swift`:
- Around line 32-40: Rename the short variable r to a descriptive identifier
(e.g. radius or halfWidth) to satisfy SwiftLint identifier_name; update all
references where r is used (center initialization: CGPoint(x: radius, y: radius)
and the radius parameter in path.addArc) and ensure start and end Angle
calculations remain unchanged; run SwiftLint/CI to confirm the violation is
resolved.
---
Duplicate comments:
In `@Sources/RunnerBar/PopoverMainView.swift`:
- Around line 42-49: The help text currently prompts the user to "add a GitHub
token" even though the button now routes to Settings; update the user-facing
copy to be neutral by changing the .help text on the unauthenticated Button (the
Button using onSelectSettings and the Circle indicator in
PopoverMainView/PopoverMainView.swift) to something like "Not authenticated —
open Settings" so it does not mention adding a GitHub token or PAT.
- Around line 43-64: The header buttons lack accessibility labels and the auth
dot is a tiny unlabeled target; update the Button for the auth dot (the branch
guarded by isAuthenticated and the Circle() view) to have a clear accessibility
label (e.g., "Not authenticated, open Settings") and a larger hit area if
needed, and add explicit .accessibilityLabel(...) to the Settings button
(Image(systemName: "gearshape") / onSelectSettings) and the Hide button
(Image(systemName: "xmark") / NSApplication.shared.hide(nil)) so VoiceOver
announces their purpose; ensure labels are concise and consider adding
.accessibilityAddTraits(.isButton) or a descriptive accessibilityHint where
helpful.
- Around line 149-175: The inline job rows currently display job.name and a
synthetic 0.5 progress when job.steps is empty; instead compute and surface the
current step title and a done/total fraction and remove the 0.5 fallback: update
the stepProgress closure (job.steps) to compute total = job.steps.count, done =
job.steps.filter { $0.conclusion != nil }.count, progress = total > 0 ?
Double(done)/Double(total) : 0.0, and derive currentStepTitle =
job.steps.first(where: { $0.conclusion == nil })?.title ??
job.steps.first?.title ?? "No steps"; replace Text(job.name) with
Text(currentStepTitle) and replace the status/label Text that currently shows
"IN PROGRESS"/"QUEUED" or the static fraction with a Text showing
"\(done)/\(total)" (or "0/0" when empty) while keeping PieProgressView(progress:
stepProgress, ... ) and existing color/size/formatting.
- Around line 197-223: The runners block is in the wrong place and displaying
the wrong trailing info; move the activeRunners block (the ForEach over
localRunners.runners) so it appears between the header and the actions list,
remove any section label, and replace the trailing
Text(runner.statusDescription) with formatted CPU% and MEM (GB) values (use the
model properties like runner.cpuUsagePercent and runner.memoryGB or compute them
from existing fields) while keeping runnerDotColor(for:), runner.runnerName and
the chevron; ensure lineLimit/truncation and padding stay the same and retain
.buttonStyle(.plain).
- Around line 31-32: The visibleCount state must be reset to the default when
the underlying actions dataset changes; add logic in PopoverMainView to set
visibleCount = 10 whenever the actions collection/property updates (e.g. use
SwiftUI's .onChange(of: actions) { _ in visibleCount = 10 } or reset in the
actions property's didSet / the view model observer). Apply the same reset
behavior where the other action lists are handled (the blocks around the code
referenced by symbols at the other ranges) so any refresh/replace of the actions
array restores the Phase 5 default and allows "No more actions" to reappear.
🪄 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: 827d0ca7-9499-4c69-853a-5056258d732a
📒 Files selected for processing (3)
Sources/RunnerBar/PieProgressView.swiftSources/RunnerBar/PopoverMainView.swiftSources/RunnerBar/RunnerStoreObservable.swift
…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.
| pop.contentSize = size | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 navigate() calls setFrameSize + contentSize while popover is shown — contradicts regression guard
The REGRESSION GUARD comment at the top of AppDelegate explicitly forbids calling setFrameSize while popover.isShown == true from outside navigate(). The new navigate() implementation now does exactly that — it calls hc.view.setFrameSize(size) and pop.contentSize = size inside a DispatchQueue.main.async block that fires while the popover IS open (popoverIsOpen is true). Per the guard comment, this was v0.25's mistake: NSPopover sees contentSize change → re-anchors X+Y → left-jump. The guard was protecting against this. The comment was updated to say 'from outside navigate()' as a carve-out, but the underlying risk (anchor drift on contentSize change while shown) remains. Recommend profiling with a large number of action rows to verify no anchor drift occurs, and add an explicit guard: only resize if the new size differs from the current contentSize by more than a threshold.
| DispatchQueue.main.async { [weak self] in | |
| guard let self, self.popoverIsOpen else { return } | |
| let fitting = hc.view.fittingSize | |
| let width = fitting.width > 0 ? fitting.width : Self.fixedWidth | |
| let height = min(max(fitting.height > 0 ? fitting.height : 300, Self.minHeight), Self.maxHeight) | |
| let newSize = NSSize(width: width, height: height) | |
| guard abs(newSize.height - (pop.contentSize.height)) > 1 || | |
| abs(newSize.width - (pop.contentSize.width)) > 1 else { return } | |
| hc.view.setFrameSize(newSize) | |
| pop.contentSize = newSize | |
| } |
AI Fix Prompt
Fix the following High (Bug) issue in `Sources/RunnerBar/AppDelegate.swift` at lines 264-272:
Problem: The REGRESSION GUARD comment at the top of AppDelegate explicitly forbids calling setFrameSize while popover.isShown == true from outside navigate(). The new navigate() implementation now does exactly that — it calls hc.view.setFrameSize(size) and pop.contentSize = size inside a DispatchQueue.main.async block that fires while the popover IS open (popoverIsOpen is true). Per the guard comment, this was v0.25's mistake: NSPopover sees contentSize change → re-anchors X+Y → left-jump. The guard was protecting against this. The comment was updated to say 'from outside navigate()' as a carve-out, but the underlying risk (anchor drift on contentSize change while shown) remains. Recommend profiling with a large number of action rows to verify no anchor drift occurs, and add an explicit guard: only resize if the new size differs from the current contentSize by more than a threshold.
Suggested fix:
DispatchQueue.main.async { [weak self] in
guard let self, self.popoverIsOpen else { return }
let fitting = hc.view.fittingSize
let width = fitting.width > 0 ? fitting.width : Self.fixedWidth
let height = min(max(fitting.height > 0 ? fitting.height : 300, Self.minHeight), Self.maxHeight)
let newSize = NSSize(width: width, height: height)
guard abs(newSize.height - (pop.contentSize.height)) > 1 ||
abs(newSize.width - (pop.contentSize.width)) > 1 else { return }
hc.view.setFrameSize(newSize)
pop.contentSize = newSize
}
| name: step.name, | ||
| status: step.status, | ||
| conclusion: step.conclusion, | ||
| startedAt: step.startedAt, |
There was a problem hiding this comment.
🟠 JobStep.startedAt / completedAt silently changed from String? to Date? — makeActiveJob call in enrichStepsIfNeeded passes raw String fields
In this diff, makeActiveJob now passes step.startedAt and step.completedAt directly (without iso.date(from:)) because the diff removes the flatMap conversion for JobStep fields. However, JobPayload.steps is typed as [JobStep]? and JobStep inherits its field types from the struct definition. If JobStep.startedAt and .completedAt were changed from String? to Date? (as suggested by the removal of iso.date(from:) calls), then JobPayload must also decode them as Date? — but JobPayload.steps is decoded from JSON which delivers ISO 8601 strings, not Date objects. This would cause a silent decode failure (steps array decoded as nil/empty) every time enrichStepsIfNeeded runs, causing step data to never populate. Verify that JobStep's Decodable conformance includes a custom date-decoding strategy, or revert the step field types to String? and restore the flatMap.
| startedAt: step.startedAt, | |
| // Option A: restore the iso conversion in makeActiveJob | |
| 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) } | |
| ) | |
| } | |
| // Option B: configure JSONDecoder with .iso8601 dateDecodingStrategy before decoding JobPayload |
AI Fix Prompt
Fix the following High (Bug) issue in `Sources/RunnerBar/ActionGroup.swift` at lines 296-312:
Problem: In this diff, makeActiveJob now passes step.startedAt and step.completedAt directly (without iso.date(from:)) because the diff removes the flatMap conversion for JobStep fields. However, JobPayload.steps is typed as [JobStep]? and JobStep inherits its field types from the struct definition. If JobStep.startedAt and .completedAt were changed from String? to Date? (as suggested by the removal of iso.date(from:) calls), then JobPayload must also decode them as Date? — but JobPayload.steps is decoded from JSON which delivers ISO 8601 strings, not Date objects. This would cause a silent decode failure (steps array decoded as nil/empty) every time enrichStepsIfNeeded runs, causing step data to never populate. Verify that JobStep's Decodable conformance includes a custom date-decoding strategy, or revert the step field types to String? and restore the flatMap.
Suggested fix:
// Option A: restore the iso conversion in makeActiveJob
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) }
)
}
// Option B: configure JSONDecoder with .iso8601 dateDecodingStrategy before decoding JobPayload
|
|
||
| /// Navigation level 1: runner status + jobs + actions. | ||
| @MainActor | ||
| private func mainView() -> AnyView { |
There was a problem hiding this comment.
🟡 enrichGroupIfNeeded makes a blocking network call on the main thread when called from onSelectAction
enrichGroupIfNeeded calls fetchActionGroups(for:) which is a blocking gh CLI invocation. The call site in onSelectAction dispatches to DispatchQueue.global(qos: .userInitiated) before calling enrichGroupIfNeeded, so the fetch itself runs on a background thread — this is correct. However, enrichGroupIfNeeded is also called internally without any background dispatch, and the function itself has no threading annotation. If a future caller invokes it on the main thread (e.g. from validatedView(for:) in openPopover), it will block the main thread for several seconds. Add a comment asserting the background-thread contract, or mark with dispatchPrecondition.
| private func mainView() -> AnyView { | |
| private func enrichGroupIfNeeded(_ group: ActionGroup) -> ActionGroup { | |
| dispatchPrecondition(condition: .notOnQueue(.main)) | |
| guard group.jobs.isEmpty else { return group } | |
| let fetched = fetchActionGroups(for: group.repo) | |
| return fetched.first(where: { $0.id == group.id }) ?? group | |
| } |
AI Fix Prompt
Fix the following Medium (Performance) issue in `Sources/RunnerBar/AppDelegate.swift` at lines 96-101:
Problem: enrichGroupIfNeeded calls fetchActionGroups(for:) which is a blocking gh CLI invocation. The call site in onSelectAction dispatches to DispatchQueue.global(qos: .userInitiated) before calling enrichGroupIfNeeded, so the fetch itself runs on a background thread — this is correct. However, enrichGroupIfNeeded is also called internally without any background dispatch, and the function itself has no threading annotation. If a future caller invokes it on the main thread (e.g. from validatedView(for:) in openPopover), it will block the main thread for several seconds. Add a comment asserting the background-thread contract, or mark with dispatchPrecondition.
Suggested fix:
private func enrichGroupIfNeeded(_ group: ActionGroup) -> ActionGroup {
dispatchPrecondition(condition: .notOnQueue(.main))
guard group.jobs.isEmpty else { return group }
let fetched = fetchActionGroups(for: group.repo)
return fetched.first(where: { $0.id == group.id }) ?? group
}
| private struct PRRef: Codable { let number: Int } | ||
|
|
||
| // MARK: - PR label | ||
|
|
There was a problem hiding this comment.
🟡 ActionGroup.Equatable conformance line exceeds line_length rule and is hard to maintain
The custom == implementation packs four conditions onto one line, exceeding the configured line_length warning threshold and making it hard to verify correctness. The runs comparison uses map({ $0.id }) == which works but allocates two arrays on every equality check — common during SwiftUI diffing. A minor readability and allocation concern.
| static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool { | |
| lhs.id == rhs.id | |
| && lhs.isDimmed == rhs.isDimmed | |
| && lhs.jobs == rhs.jobs | |
| && lhs.runs.map(\.id) == rhs.runs.map(\.id) | |
| } |
AI Fix Prompt
Fix the following Medium (Style) issue in `Sources/RunnerBar/ActionGroup.swift` at line 172:
Problem: The custom == implementation packs four conditions onto one line, exceeding the configured line_length warning threshold and making it hard to verify correctness. The runs comparison uses map({ $0.id }) == which works but allocates two arrays on every equality check — common during SwiftUI diffing. A minor readability and allocation concern.
Suggested fix:
static func == (lhs: ActionGroup, rhs: ActionGroup) -> Bool {
lhs.id == rhs.id
&& lhs.isDimmed == rhs.isDimmed
&& lhs.jobs == rhs.jobs
&& lhs.runs.map(\.id) == rhs.runs.map(\.id)
}
| case ..<3_600: return "\(Int(seconds / 60))m ago" | ||
| case ..<172_800: return "\(Int(seconds / 3_600))h ago" | ||
| default: return "\(Int(seconds / 86_400))d ago" | ||
| } |
There was a problem hiding this comment.
🔵 RelativeTimeFormatter 172_800 boundary skips 2-day display — jumps straight from hours to days
The switch uses ..<172_800 (48h) for the hours bucket, meaning at exactly 48h it switches to days (showing '2d ago'). This is fine at the boundary, but the range ..<3_600 → ..<172_800 means any value between 1h and 48h shows in hours (e.g. '36h ago') rather than switching to days at 24h. Most relative-time formatters switch to days at 24h. This is a minor UX inconsistency — '27h ago' reads oddly compared to '1d ago'. Consider using 86_400 (24h) as the hours/days boundary.
| } | |
| switch seconds { | |
| case ..<60: return "just now" | |
| case ..<3_600: return "\(Int(seconds / 60))m ago" | |
| case ..<86_400: return "\(Int(seconds / 3_600))h ago" | |
| default: return "\(Int(seconds / 86_400))d ago" | |
| } |
AI Fix Prompt
Fix the following Low (Logic Error) issue in `Sources/RunnerBar/RelativeTimeFormatter.swift` at lines 27-30:
Problem: The switch uses ..<172_800 (48h) for the hours bucket, meaning at exactly 48h it switches to days (showing '2d ago'). This is fine at the boundary, but the range ..<3_600 → ..<172_800 means any value between 1h and 48h shows in hours (e.g. '36h ago') rather than switching to days at 24h. Most relative-time formatters switch to days at 24h. This is a minor UX inconsistency — '27h ago' reads oddly compared to '1d ago'. Consider using 86_400 (24h) as the hours/days boundary.
Suggested fix:
switch seconds {
case ..<60: return "just now"
case ..<3_600: return "\(Int(seconds / 60))m ago"
case ..<86_400: return "\(Int(seconds / 3_600))h ago"
default: return "\(Int(seconds / 86_400))d ago"
}
|
|
||
| // MARK: - ActionsListView | ||
|
|
||
| /// Actions list with per-group expand/collapse and pagination (Phase 3–5 / #302 #304 #305). |
There was a problem hiding this comment.
🔵 NSApplication.shared.hide(nil) in PopoverHeaderView closes the app, not just the popover
The close (✕) button in PopoverHeaderView calls NSApplication.shared.hide(nil), which hides the entire application (equivalent to Cmd+H). For a menu bar app, the expected behaviour of a close/dismiss button is to close the popover only. The correct call is popover.performClose(nil) or going through a callback. Since PopoverHeaderView doesn't have a reference to the popover, the cleanest fix is to pass an onClose callback from AppDelegate, similar to how onSelectSettings is passed.
| /// Actions list with per-group expand/collapse and pagination (Phase 3–5 / #302 #304 #305). | |
| // In PopoverHeaderView, add: | |
| let onClose: () -> Void | |
| // Replace the Button action: | |
| Button( | |
| action: onClose, | |
| label: { | |
| Image(systemName: "xmark") | |
| .font(.system(size: 11)) | |
| .foregroundColor(.secondary) | |
| } | |
| ) | |
| .buttonStyle(.plain) | |
| .help("Close popover") | |
| // In AppDelegate.mainView(), pass: | |
| PopoverHeaderView( | |
| systemStats: systemStats, | |
| isAuthenticated: isAuthenticated, | |
| onSelectSettings: onSelectSettings, | |
| onClose: { [weak self] in self?.popover?.performClose(nil) } | |
| ) |
AI Fix Prompt
Fix the following Low (Bug) issue in `Sources/RunnerBar/PopoverMainView.swift` at lines 168-174:
Problem: The close (✕) button in PopoverHeaderView calls NSApplication.shared.hide(nil), which hides the entire application (equivalent to Cmd+H). For a menu bar app, the expected behaviour of a close/dismiss button is to close the popover only. The correct call is popover.performClose(nil) or going through a callback. Since PopoverHeaderView doesn't have a reference to the popover, the cleanest fix is to pass an onClose callback from AppDelegate, similar to how onSelectSettings is passed.
Suggested fix:
// In PopoverHeaderView, add:
let onClose: () -> Void
// Replace the Button action:
Button(
action: onClose,
label: {
Image(systemName: "xmark")
.font(.system(size: 11))
.foregroundColor(.secondary)
}
)
.buttonStyle(.plain)
.help("Close popover")
// In AppDelegate.mainView(), pass:
PopoverHeaderView(
systemStats: systemStats,
isAuthenticated: isAuthenticated,
onSelectSettings: onSelectSettings,
onClose: { [weak self] in self?.popover?.performClose(nil) }
)
| Circle() | ||
| .fill(jobDotColor(for: job)) | ||
| .frame(width: 8, height: 8) | ||
| var body: some View { |
There was a problem hiding this comment.
🔵 RunnersListView filters to busy-only but displays empty state misleadingly when all runners are idle
RunnersListView now filters runners to only those that are busy (actively running a job). When all runners are idle, the filtered list is empty and the section either disappears entirely or shows an empty state. A user seeing no runners listed may incorrectly conclude that no runners are connected or that the fetch failed, rather than understanding that all runners are idle. The previous behaviour showed all runners. At minimum, the empty state should clarify 'No busy runners — all runners idle' rather than showing nothing or a generic empty message.
| var body: some View { | |
| // In RunnersListView, after filtering: | |
| let busyRunners = runners.filter { $0.busy } | |
| if busyRunners.isEmpty { | |
| Text("All runners idle") | |
| .font(.caption) | |
| .foregroundColor(.secondary) | |
| .frame(maxWidth: .infinity, alignment: .center) | |
| .padding(.vertical, 4) | |
| } else { | |
| ForEach(busyRunners) { runner in | |
| RunnerRowView(runner: runner) | |
| } | |
| } |
AI Fix Prompt
Fix the following Low (UX) issue in `Sources/RunnerBar/PopoverMainView.swift` at lines 220-235:
Problem: RunnersListView now filters runners to only those that are busy (actively running a job). When all runners are idle, the filtered list is empty and the section either disappears entirely or shows an empty state. A user seeing no runners listed may incorrectly conclude that no runners are connected or that the fetch failed, rather than understanding that all runners are idle. The previous behaviour showed all runners. At minimum, the empty state should clarify 'No busy runners — all runners idle' rather than showing nothing or a generic empty message.
Suggested fix:
// In RunnersListView, after filtering:
let busyRunners = runners.filter { $0.busy }
if busyRunners.isEmpty {
Text("All runners idle")
.font(.caption)
.foregroundColor(.secondary)
.frame(maxWidth: .infinity, alignment: .center)
.padding(.vertical, 4)
} else {
ForEach(busyRunners) { runner in
RunnerRowView(runner: runner)
}
}
| .resolvingSymlinksInPath.path | ||
| .resolvingSymlinksInPath().path | ||
| guard resolvedDir.hasPrefix(homeDir) else { | ||
| DispatchQueue.main.async { |
There was a problem hiding this comment.
💡 Removed security comment for path-traversal guard reduces future maintainability
The diff removes the comment block explaining WHY the homeDir path-traversal guard exists ('A freeform path like ~/../../usr/local/bin could otherwise cause an arbitrary executable to be launched with the user's privileges'). The guard itself is preserved, which is good. But without the comment, a future maintainer might remove the guard thinking it is overly cautious. The comment is the only documentation of this security rationale.
| DispatchQueue.main.async { | |
| // Security: validate that installDir resolves inside the user's home directory | |
| // before executing config.sh there. A path like ~/../../usr/bin could otherwise | |
| // launch an arbitrary executable with the user's privileges. | |
| let homeDir = FileManager.default.homeDirectoryForCurrentUser | |
| .resolvingSymlinksInPath().path |
AI Fix Prompt
Fix the following Nit (Security) issue in `Sources/RunnerBar/AddRunnerSheet.swift` at lines 161-164:
Problem: The diff removes the comment block explaining WHY the homeDir path-traversal guard exists ('A freeform path like ~/../../usr/local/bin could otherwise cause an arbitrary executable to be launched with the user's privileges'). The guard itself is preserved, which is good. But without the comment, a future maintainer might remove the guard thinking it is overly cautious. The comment is the only documentation of this security rationale.
Suggested fix:
// Security: validate that installDir resolves inside the user's home directory
// before executing config.sh there. A path like ~/../../usr/bin could otherwise
// launch an arbitrary executable with the user's privileges.
let homeDir = FileManager.default.homeDirectoryForCurrentUser
.resolvingSymlinksInPath().path
…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.
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
… 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.
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
eonist
force-pushed
the
feature/296-popover-main-view-redesign
branch
from
1edb98c to
1d56db5
Compare
#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)
|
1 participant
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 with live system stats, richer action history, and inline job details
What Changed
Impact
✅ Faster scan of recent workflow activity✅ Clearer live status for running jobs and actions✅ Safer runner registration paths🔄 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.