fix: scope refresh-row disabling to the provider being refreshed#1911
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 10:27 AM ET / 14:27 UTC. Summary Reproducibility: yes. from source inspection: current main checks a single manualRefreshTask before provider-specific state, so any manual refresh makes every persistent Refresh row look busy. I did not run live app validation because this was a read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the scoped refresh-task implementation once the PR body includes clear redacted proof showing one provider refreshing while unrelated provider Refresh rows remain enabled. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: current main checks a single manualRefreshTask before provider-specific state, so any manual refresh makes every persistent Refresh row look busy. I did not run live app validation because this was a read-only review. Is this the best way to solve the issue? Yes, the scoped task map with explicit global/provider exclusivity is the narrow maintainable fix for the reported menu-state bug. The remaining gap is proof quality, not a visible code defect. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9ad35deb6a89. 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
Review history (5 earlier review cycles)
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the status menu’s “Refresh” row gating so manual refresh is scoped to the provider being refreshed (instead of disabling refresh UI globally), while still supporting a true global refresh for ⌘R / merged overview.
Changes:
- Replace the single global
manualRefreshTaskwith scopedmanualRefreshTaskskeyed by.globalvs.provider(UsageProvider). - Update refresh-in-flight checks so only the active provider’s menu/tab greys out during its own refresh (global refresh still greys all).
- Update
MenuCardRefreshMonitorand refresh-row tests to reflect per-provider manual refresh behavior and allow concurrent provider refreshes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuSwitcherRefreshTests.swift | Adds coverage ensuring provider refresh only greys its own tab and global refresh greys all. |
| Tests/CodexBarTests/StatusMenuPersistentRefreshTests.swift | Reworks refresh gating assertions and adds concurrency test for parallel provider refresh starts. |
| Sources/CodexBar/StatusItemController+Shutdown.swift | Cancels and clears all scoped manual refresh tasks on shutdown; resets monitor state. |
| Sources/CodexBar/StatusItemController+PersistentMenuActions.swift | Scopes isRefreshActionInFlight(for:) to the provider’s own manual refresh task (or global). |
| Sources/CodexBar/StatusItemController+MenuTracking.swift | Treats any scoped manual refresh task as “menu data refresh in flight.” |
| Sources/CodexBar/StatusItemController+Actions.swift | Introduces ManualRefreshScope and stores manual refresh tasks per scope when starting refresh. |
| Sources/CodexBar/StatusItemController.swift | Replaces manualRefreshTask/manualRefreshProvider storage with scoped manualRefreshTasks plus test accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
clawsweeper
Bot
added
proof: 📸 screenshot
|
Updated the PR body with local menu-state proof from @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
steipete
force-pushed
the
fix/scoped-refresh-row-greying
branch
from
b7b7c20 to
f562916
Compare
clawsweeper
Bot
added
proof: 📸 screenshot
Refreshing one provider greyed out — and single-flighted — the Refresh row for every provider, so the menu implied the others were refreshing when they were not. Replace the single global `manualRefreshTask` with a per-scope map (`.global` for the all-providers ⌘R/overview refresh, `.provider(p)` for a single provider). The disable gate now reflects only the target provider's own refresh (a global refresh still disables every row), and `startManualRefresh` guards per scope so different providers can refresh concurrently instead of one blocking the rest. `MenuCardRefreshMonitor` tracks in-flight providers as a set plus a global flag. Rewrites the refresh-row tests that encoded the old blanket behavior and adds coverage for scoped greying and concurrent per-provider refreshes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er overlap - MenuCardRefreshMonitor: keep an already-refreshing provider's frozen card when a second provider's refresh re-supplies models (old-wins merge), so the first card no longer jumps mid-refresh — the artifact the freeze exists to prevent. - startManualRefresh: make a .global refresh mutually exclusive with any per-provider one (and vice versa); two different providers still refresh concurrently. - Merged-overview Refresh row now reflects any in-flight manual refresh, including a per-provider refresh's post-fetch status/token tail, instead of flickering. - Make the test-only manualRefreshTask / manualRefreshProvider accessors return a value only when unambiguous (global, or exactly one task), else nil; move them to +Actions. - Add tests: concurrent frozen-card retention, and overview-busy-through-tail + global refresh exclusion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ard, scoped overview greying
steipete
force-pushed
the
fix/scoped-refresh-row-greying
branch
from
d57954e to
ea52f67
Compare
|
Landed after maintainer hardening. Thank you, @hhh2210! Final proof on exact contributor head
The landed version scopes in-flight refresh state and frozen menu snapshots per provider, preserves global-refresh exclusivity, permits independent provider refreshes, rejects stale completions, and adds regression coverage that distinguishes each provider's own refresh-start snapshot. Squash merge: Changelog credit: |
Problem
Refreshing one provider disabled the Refresh row for every provider. The controller also used one global manual-refresh task, so a second provider refresh was silently dropped while the first ran.
Change
Validation
swift test --filter 'StatusMenuPersistentRefreshTests|StatusMenuSwitcherRefreshTests|StatusMenuMergedOverviewRefreshTests|StatusMenuClosedPreparationTests'(55 tests)make test(47 shards)make checkVisual proof
Refreshing Claude leaves unrelated provider rows enabled: