Skip to content

Revert "🎨 Redesign PopoverMainView β€” unified scrollable actions list (issue #294)"#312

Merged
eonist merged 1 commit into
mainfrom
revert-309-fix/issue-294-popover-redesign
May 8, 2026
Merged

Revert "🎨 Redesign PopoverMainView β€” unified scrollable actions list (issue #294)"#312
eonist merged 1 commit into
mainfrom
revert-309-fix/issue-294-popover-redesign

Conversation

@eonist
Copy link
Copy Markdown
Collaborator

@eonist eonist commented May 8, 2026

Reverts #309

Summary by CodeRabbit

  • Refactor
    • Streamlined the RunnerBar popover interface by simplifying the UI layout and removing action pagination and job expansion features. The popover now displays up to 5 actions and 3 active jobs with a more linear, consolidated view structure.

@codeant-ai
Copy link
Copy Markdown

codeant-ai Bot commented May 8, 2026

Skipping CodeAnt AI review β€” this PR is a revert. The diff is the inverse of code that was previously reviewed and merged, so a fresh review typically adds no signal.

If you want CodeAnt to double-check the revert, comment @codeant-ai : review and CodeAnt will start a review.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6224210e-fba3-4e71-9b84-0f5b7c7d5d2d

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 2aa5e95 and c6b6e83.

πŸ“’ Files selected for processing (2)
  • Sources/RunnerBar/PopoverMainView.swift
  • Sources/RunnerBar/PopoverMainViewSubviews.swift

πŸ“ Walkthrough

Walkthrough

PopoverMainView.swift was refactored to consolidate UI rendering and state management into a single inline body, removing the previous pagination and inline-job-expansion logic. PopoverMainViewSubviews.swift was deleted entirely. Helper methods for status colors and labels are now defined directly in the main view.

Changes

UI Consolidation & Pagination Removal

Layer / File(s) Summary
View Simplification
Sources/RunnerBar/PopoverMainView.swift
Replaced separate composed views and complex state (expandedGroupIDs, visibleCount, pagination toggles) with a unified inline VStack layout. New body includes header (version, settings button, GitHub auth), optional rate-limit banner, "System" stats section, "Actions" section (max 5 items), "Active Jobs" section (max 3 items), and quit button.
Helper Methods
Sources/RunnerBar/PopoverMainView.swift
Added local helper methods directly in the view: actionDotColor, jobDotColor, jobStatusLabel, conclusionLabel, and conclusionColor for status and conclusion mapping.
Subview Deletion
Sources/RunnerBar/PopoverMainViewSubviews.swift
Deleted 316 lines; all subview definitions (prior header, action/job rows, expansion logic) consolidated into main view body.
Lifecycle & Auth
Sources/RunnerBar/PopoverMainView.swift
Removed onDisappear handler for systemStats stop; signInWithGitHub() retained with updated comments reflecting token resolution approach.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • eoncode/runner-bar#244 β€” Both modify PopoverMainView.swift to add settings button and remove scope/runner UI alongside pagination/expansion logic.
  • eoncode/runner-bar#309 β€” Inverse changes: that PR adds pagination and expandable action groups with extracted subviews; this PR removes pagination/expansion and inlines all helpers.

Suggested labels

size:L

Poem

🐰 The views have consolidated, compact and clean,
Pagination vanished, no expand-shrink scene,
Helpers inline, status colors flow bright,
One unified body shines simpler and light!

✨ Finishing Touches
πŸ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert-309-fix/issue-294-popover-redesign

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Revert PopoverMainView redesign to restore stable layout

🐞 Bug fix

Grey Divider

Walkthroughs

Description
β€’ Reverts PR #309 redesign that introduced regressions
β€’ Restores inline popover layout with system stats, actions, jobs sections
β€’ Removes complex action expansion/pagination logic
β€’ Consolidates subview components back into main file
Diagram
flowchart LR
  A["PR #309 Redesign<br/>with ScrollView"] -->|Revert| B["Inline Layout<br/>with Sections"]
  B --> C["System Stats"]
  B --> D["Actions List"]
  B --> E["Active Jobs"]
  B --> F["Quit Button"]
Loading

Grey Divider

File Changes

1. Sources/RunnerBar/PopoverMainView.swift 🐞 Bug fix +205/-93

Restore inline sectioned popover layout from ScrollView design

β€’ Reverts from ScrollView-based unified layout back to inline sectioned layout
β€’ Removes visibleCount and expandedGroupIDs state variables for pagination/expansion
β€’ Restores inline header with version, settings button, and auth indicator
β€’ Inlines all UI sections (System, Actions, Active Jobs) directly in body
β€’ Removes dependency on PopoverMainViewSubviews.swift components
β€’ Adds helper functions for status colors and labels (actionDot, jobDot, jobStatusLabel, etc.)
β€’ Adds swiftlint:disable type_body_length directive to accommodate inline code

Sources/RunnerBar/PopoverMainView.swift


2. Sources/RunnerBar/PopoverMainViewSubviews.swift Miscellaneous +0/-316

Delete subviews file after consolidation

β€’ File completely deleted as all subview components are inlined back into PopoverMainView.swift
β€’ Removes PopoverHeaderView, PopoverLocalRunnerRow, ActionRowView, and InlineJobRowsView
β€’ Eliminates complex action group expansion and inline job row display logic

Sources/RunnerBar/PopoverMainViewSubviews.swift


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 8, 2026

Code Review by Qodo

🐞 Bugs (3) πŸ“˜ Rule violations (0)

Grey Divider


Action required

1. Undefined systemStats.start() 🐞 Bug ≑ Correctness
Description
PopoverMainView calls systemStats.start(), but SystemStatsViewModel does not define a start()
method, causing a compilation failure. This blocks the app from building.
Code

Sources/RunnerBar/PopoverMainView.swift[R173-176]

        .onAppear {
            isAuthenticated = (githubToken() != nil)
            systemStats.start()
        }
-        // Fix #4: tear down the repeating timer/publisher on dismiss to prevent
-        // SystemStatsViewModel leaking CPU cycles after the view is gone.
-        .onDisappear {
-            systemStats.stop()
-        }
Evidence
PopoverMainView invokes systemStats.start() on appear, but SystemStatsViewModel only defines
init/deinit and internal sampling methods; no start() API exists on the type, so the call cannot
compile.

Sources/RunnerBar/PopoverMainView.swift[172-176]
Sources/RunnerBar/SystemStats.swift[70-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PopoverMainView` calls `systemStats.start()`, but `SystemStatsViewModel` has no `start()` method, which will fail compilation.

### Issue Context
`SystemStatsViewModel` currently creates its repeating `Timer` in `init()`. You can either (a) add a `start()` API (and likely move timer creation out of `init()`), or (b) remove the call to `start()` and rely on `init()` (not recommended because it keeps polling even when popover is closed).

### Fix Focus Areas
- Sources/RunnerBar/PopoverMainView.swift[172-176]
- Sources/RunnerBar/SystemStats.swift[70-86]

β“˜ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. System stats timer always runs 🐞 Bug ➹ Performance
Description
SystemStatsViewModel starts a repeating 2-second Timer in init(), and AppDelegate constructs
PopoverMainView as the hosting controller’s root view at launch, so sampling continues even when the
popover is closed. This creates unnecessary background CPU work for the lifetime of the app.
Code

Sources/RunnerBar/PopoverMainView.swift[R29-31]

    @State private var isAuthenticated = (githubToken() != nil)
    @StateObject private var systemStats = SystemStatsViewModel()
-    @State private var visibleCount: Int = 10
-    @State private var expandedGroupIDs: Set<String> = []
Evidence
SystemStatsViewModel schedules a repeating Timer during initialization. AppDelegate creates an
NSHostingController with rootView = mainView() during app launch, and mainView() constructs
PopoverMainView, which instantiates @StateObject systemStats = SystemStatsViewModel(), starting
the timer even before the popover is shown.

Sources/RunnerBar/SystemStats.swift[77-83]
Sources/RunnerBar/AppDelegate.swift[59-69]
Sources/RunnerBar/AppDelegate.swift[106-131]
Sources/RunnerBar/PopoverMainView.swift[29-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
System stats polling starts in `SystemStatsViewModel.init()` and therefore runs continuously, including when the popover is not visible.

### Issue Context
The app builds `PopoverMainView` as the hosting controller’s `rootView` at launch, so any `@StateObject` initialized inside it will be created immediately.

### Fix Focus Areas
- Sources/RunnerBar/SystemStats.swift[77-83]
- Sources/RunnerBar/PopoverMainView.swift[172-177]

### Suggested approach
1. Refactor `SystemStatsViewModel` so `init()` does not create a repeating timer.
2. Add explicit `start()` (create/schedule timer) and `stop()` (invalidate timer, set to nil) methods.
3. In `PopoverMainView`, call `systemStats.start()` in `.onAppear` and `systemStats.stop()` in `.onDisappear` so polling only happens while the popover is visible.

β“˜ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Hardcoded version string 🐞 Bug βš™ Maintainability
Description
The popover header hardcodes a specific version string ("RunnerBar v0.34"), which will become
incorrect as soon as the bundle version changes. The codebase already reads the app version/build
from Bundle.main elsewhere.
Code

Sources/RunnerBar/PopoverMainView.swift[R35-37]

+            HStack {
+                Text("RunnerBar v0.34") // ⚠️ bump on every commit
+                    .font(.headline).foregroundColor(.secondary)
Evidence
PopoverMainView uses a fixed version string instead of reading the version from the app bundle;
SettingsView already has helpers for reading CFBundleShortVersionString/CFBundleVersion.

Sources/RunnerBar/PopoverMainView.swift[35-37]
Sources/RunnerBar/SettingsView.swift[51-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The header text hardcodes the app version, which will drift from the actual bundle version.

### Issue Context
`SettingsView` already reads version/build from `Bundle.main.infoDictionary`.

### Fix Focus Areas
- Sources/RunnerBar/PopoverMainView.swift[35-37]
- Sources/RunnerBar/SettingsView.swift[51-57]

### Suggested change
Replace `Text("RunnerBar v0.34")` with a value derived from `CFBundleShortVersionString` (and optionally `CFBundleVersion`).

β“˜ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2026

@eonist eonist merged commit 6638a49 into main May 8, 2026
3 of 5 checks passed
Comment on lines 173 to 176
.onAppear {
isAuthenticated = (githubToken() != nil)
systemStats.start()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Undefined systemstats.start() 🐞 Bug ≑ Correctness

PopoverMainView calls systemStats.start(), but SystemStatsViewModel does not define a start()
method, causing a compilation failure. This blocks the app from building.
Agent Prompt
### Issue description
`PopoverMainView` calls `systemStats.start()`, but `SystemStatsViewModel` has no `start()` method, which will fail compilation.

### Issue Context
`SystemStatsViewModel` currently creates its repeating `Timer` in `init()`. You can either (a) add a `start()` API (and likely move timer creation out of `init()`), or (b) remove the call to `start()` and rely on `init()` (not recommended because it keeps polling even when popover is closed).

### Fix Focus Areas
- Sources/RunnerBar/PopoverMainView.swift[172-176]
- Sources/RunnerBar/SystemStats.swift[70-86]

β“˜ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant