feat: show in-flight feedback on manual refresh - #1458
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 12, 2026, 3:51 AM ET / 07:51 UTC. Summary Reproducibility: yes. Current main defers rebuilding an open tracked menu, so already-hosted cards and the persistent Refresh row remain visually stale during manual refresh; the exact-head live proof demonstrates the corrected success and failure paths. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Merge the exact head through ordinary maintainer handling, retaining the in-place hosted-view updates, canonical subtitle resolution, fixed spinner geometry, single-task coalescing, shutdown cleanup, and focused lifecycle coverage. Do we have a high-confidence way to reproduce the issue? Yes. Current main defers rebuilding an open tracked menu, so already-hosted cards and the persistent Refresh row remain visually stale during manual refresh; the exact-head live proof demonstrates the corrected success and failure paths. Is this the best way to solve the issue? Yes. Updating the existing SwiftUI-hosted subtitle and fixed-size AppKit action row in place is the narrowest maintainable approach because it preserves the no-rebuild-during-tracking boundary and reuses canonical provider subtitle and error resolution. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f759e119e4f8. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds in-place “refresh in progress” UI updates (spinner + “Refreshing…” subtitle) while the status menu stays open, avoiding NSMenu rebuilds during tracking.
Changes:
- Track live persistent “Refresh” rows and toggle a spinner immediately on click, then sync to
store.isRefreshing. - Inject a refresh-state monitor into SwiftUI menu card views via
EnvironmentValuesto update subtitles in place. - Add tests covering fixed row metrics, row/store sync behavior, and refresh-indicator gating.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuPersistentRefreshTests.swift | Adds tests for persistent refresh spinner metrics and store-sync behavior. |
| Sources/CodexBar/StatusItemController.swift | Introduces refresh monitor + weak tracking of persistent refresh rows; sync on store observation. |
| Sources/CodexBar/StatusItemController+PersistentMenuActions.swift | Adds APIs to force/sync spinner state across tracked refresh rows. |
| Sources/CodexBar/StatusItemController+MenuPresentation.swift | Adds spinner overlay to persistent action row; injects refresh monitor into SwiftUI environment. |
| Sources/CodexBar/StatusItemController+MenuCardItems.swift | Passes refresh monitor into menu card container view. |
| Sources/CodexBar/StatusItemController+Menu.swift | Registers refresh rows and initializes spinner state during menu build. |
| Sources/CodexBar/StatusItemController+Actions.swift | Forces spinner on immediately when refresh is triggered. |
| Sources/CodexBar/MenuHighlightStyle.swift | Adds menuCardRefreshMonitor environment entry. |
| Sources/CodexBar/MenuCardView.swift | Adds MenuCardRefreshMonitor and live subtitle override based on refresh state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Clicking the persistent Refresh row (⌘R) gave no immediate feedback while the menu stayed open: the provider card kept showing "Updated …" and only changed after switching cards or reopening the menu, prompting repeat clicks. Add two in-place updates that work during NSMenu tracking without rebuilding the open menu (preserving the deferred parent-rebuild policy from steipete#1001/steipete#1196/ steipete#1325): - Live card subtitle: a narrowly-scoped MenuCardRefreshMonitor wraps the @observable UsageStore and is injected via the environment. The header subtitle reads shouldShowRefreshingMenuCardIndicator(for:) so SwiftUI re-renders the single-line subtitle in place to "Refreshing…" while a refresh is in flight, converging to the next rebuild's state. The gate requires error == nil, so the multi-line error layout is never swapped and row height stays fixed. - Refresh row spinner: PersistentMenuActionItemView swaps its arrow.clockwise icon for a spinner occupying the same fixed 18pt slot (icon faded, not hidden, to avoid stack collapse). Forced on at click for instant feedback and reverted via the store observation on completion/failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
74ff56c to
fec70d6
Compare
fec70d6 to
2ced0e6
Compare
|
Exact-head maintainer proof for
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Problem
Clicking Refresh while the menu is open gives no immediate feedback. The visible provider cards keep their old subtitles and the persistent Refresh row looks idle until the network round-trip completes. Because tracked root menus intentionally defer rebuilds, the existing refreshing state was not reaching already-hosted views.
That ambiguity also encourages repeated clicks, which could start overlapping manual refresh work.
Fix
Surface one explicit manual-refresh lifecycle in place, without rebuilding the tracked menu:
MenuCardRefreshMonitorinto hosted provider cardsRefreshing…, then surface fresh success or failure subtitles when the request finishesThe updates mutate existing SwiftUI/AppKit views only; tracked
NSMenugeometry remains stable.Tests
StatusMenuPersistentRefreshTestsnow covers 13 cases, including:Verification
swift test --filter StatusMenuPersistentRefreshTests(13/13)make check./Scripts/compile_and_run.shRefreshing…, one fixed-slot spinner remains, geometry stays stableProof