Add weekly menubar metric preference#555
Open
michaljuris wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a "weekly menubar metric" setting that lets users choose between "default" and "weekly" progress views in the tray icon. The feature includes plugin manifest schema updates for marking weekly lines, backend parsing to expose the weekly candidate, frontend settings UI, and tray display logic that uses the preference to select which progress bar to show. ChangesWeekly Menubar Metric Feature
Sequence DiagramsequenceDiagram
participant User
participant SettingsPage as Settings UI
participant DisplayActions
participant Store as App Store
participant TrayIcon
participant PluginMeta as Backend
User->>SettingsPage: Select "Weekly" metric
SettingsPage->>DisplayActions: onMenubarMetricChange("weekly")
DisplayActions->>Store: setMenubarMetric("weekly")
DisplayActions->>DisplayActions: saveMenubarMetric("weekly")
DisplayActions->>TrayIcon: Trigger update
TrayIcon->>Store: Read menubarMetric
TrayIcon->>PluginMeta: Use weeklyCandidate if available
TrayIcon->>TrayIcon: getTrayPrimaryBars(preferWeekly=true)
TrayIcon->>TrayIcon: formatTrayTooltip(bars, weeklyMode=true)
TrayIcon->>User: Render weekly progress bars & tooltip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
github-actions
Bot
added
rust
michaljuris
force-pushed
the
feat/menubar-weekly-metric
branch
2 times, most recently
from
4e03de7 to
b028041
Compare
12 tasks
Add a global Metric toggle (Default | Weekly) under Settings -> Menubar Icon that switches the tray icon and tooltip to a provider's weekly usage when one is available. Providers declare their weekly line in plugin.json via "period": "weekly". The Rust manifest loader resolves it into weeklyCandidate (mirroring the existing primaryCandidates), so tray selection stays declarative instead of matching display-label text. Providers without a weekly line fall back to their primary metric, and in weekly mode the tooltip tags lines with their metric label only when the list is mixed. Refs robinebers#393
michaljuris
force-pushed
the
feat/menubar-weekly-metric
branch
from
b028041 to
90e4dc3
Compare
michaljuris
requested review from
davidarny,
robinebers and
validatedev
as code owners
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plugins/schema.md (1)
278-278:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate example to use new
primaryOrderschema.This example still uses the deprecated
"primary": trueboolean syntax. It should be updated to match the newprimaryOrdernumeric sorting mechanism documented above.📝 Proposed fix
- { "type": "progress", "label": "Usage", "scope": "overview", "primary": true }, + { "type": "progress", "label": "Usage", "scope": "overview", "primaryOrder": 1 },🤖 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 `@docs/plugins/schema.md` at line 278, Update the example JSON object that currently uses the deprecated "primary": true flag to the new numeric sorting key: remove the "primary" property and add "primaryOrder": 0 (or the appropriate numeric rank) to indicate its priority; target the JSON entry with keys "type": "progress", "label": "Usage", "scope": "overview" and replace the boolean primary with the numeric primaryOrder.
🧹 Nitpick comments (3)
src/lib/tray-primary-progress.test.ts (1)
92-92: 💤 Low valueConsider explicitly asserting
label: undefinedfor consistency.Other updated test expectations in this file now explicitly include the
labelfield (e.g., lines 166, 203, 241, 286). For consistency and clarity, this test should also assertlabel: undefinedto make the expected output shape explicit when no data is available.Proposed update for consistency
- expect(bars).toEqual([{ id: "a", fraction: undefined }]) + expect(bars).toEqual([{ id: "a", fraction: undefined, label: undefined }])🤖 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 `@src/lib/tray-primary-progress.test.ts` at line 92, Update the test expectation to explicitly include the label field so the shape matches other assertions: change the toEqual assertion that checks bars (currently comparing to [{ id: "a", fraction: undefined }]) to include label: undefined as well; locate the assertion referencing bars in tray-primary-progress.test (the expect(bars).toEqual(...) line) and add label: undefined to the expected object to match the explicit expectations used elsewhere.src/hooks/app/use-settings-display-actions.test.ts (1)
115-156: 💤 Low valueConsider adding menubar metric persistence failure coverage.
The "logs persistence failures" test verifies error logging for theme, display, reset timer, and time format save failures, but omits menubar metric. For consistency, consider adding a failure case for
saveMenubarMetricMockand asserting the correspondingconsole.errorcall.🧪 Proposed test extension
it("logs persistence failures", async () => { const themeError = new Error("theme failed") const displayError = new Error("display failed") const resetError = new Error("reset failed") + const metricError = new Error("metric failed") const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) saveThemeModeMock.mockRejectedValueOnce(themeError) saveDisplayModeMock.mockRejectedValueOnce(displayError) saveResetTimerDisplayModeMock.mockRejectedValueOnce(resetError) + saveMenubarMetricMock.mockRejectedValueOnce(metricError) const timeFormatError = new Error("time format failed") saveTimeFormatModeMock.mockRejectedValueOnce(timeFormatError) const { result } = renderHook(() => useSettingsDisplayActions({ setThemeMode: vi.fn(), setDisplayMode: vi.fn(), resetTimerDisplayMode: "relative", setResetTimerDisplayMode: vi.fn(), setTimeFormatMode: vi.fn(), setMenubarIconStyle: vi.fn(), setMenubarMetric: vi.fn(), scheduleTrayIconUpdate: vi.fn(), }) ) act(() => { result.current.handleThemeModeChange("light") result.current.handleDisplayModeChange("left") result.current.handleResetTimerDisplayModeChange("relative") result.current.handleTimeFormatModeChange("12h") + result.current.handleMenubarMetricChange("weekly") }) await waitFor(() => { expect(errorSpy).toHaveBeenCalledWith("Failed to save theme mode:", themeError) expect(errorSpy).toHaveBeenCalledWith("Failed to save display mode:", displayError) expect(errorSpy).toHaveBeenCalledWith("Failed to save reset timer display mode:", resetError) expect(errorSpy).toHaveBeenCalledWith("Failed to save time format mode:", timeFormatError) + expect(errorSpy).toHaveBeenCalledWith("Failed to save menubar metric:", metricError) }) errorSpy.mockRestore() })🤖 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 `@src/hooks/app/use-settings-display-actions.test.ts` around lines 115 - 156, Add a failing persistence case for the menubar metric in the "logs persistence failures" test: have saveMenubarMetricMock.mockRejectedValueOnce(new Error("menubar metric failed")), invoke result.current.handleMenubarMetricChange(...) inside the act block (use the same pattern as handleThemeModeChange/handleDisplayModeChange), and add an expectation in the waitFor that console.error was called with the matching message ("Failed to save menubar metric:") and the error; this updates the test around useSettingsDisplayActions, handleMenubarMetricChange, and saveMenubarMetricMock to assert menubar metric failure logging.docs/plugins/schema.md (1)
127-141: ⚡ Quick winConsider clarifying that weekly lines may optionally have
primaryOrder.The docs state that "the provider must still define a primary (
primaryOrder) line" but don't explicitly address whether the weekly line itself can haveprimaryOrder. Looking at the plugin configs:
claude/plugin.jsonline 15: Weekly line hasprimaryOrder: 2(serves as both weekly candidate and secondary primary)devin/plugin.jsonline 10: Weekly quota hasprimaryOrder: 1(weekly IS the primary)codex/plugin.jsonline 15: Weekly line has NOprimaryOrder(weekly-only, not a primary candidate)All three patterns are valid and work correctly. A sentence clarifying "A weekly line may optionally include
primaryOrderto also serve as a primary candidate" would eliminate ambiguity.🤖 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 `@docs/plugins/schema.md` around lines 127 - 141, Add a clarifying sentence to the Weekly Metric section: state that a line with "period": "weekly" may optionally include a primaryOrder value to also serve as a primary candidate (so a weekly line can be both the weekly override and count toward primary ordering), and keep the existing rule that a provider must still define at least one primaryOrder line to appear in the menubar; reference the JSON keys "lines", "period", and "primaryOrder" when explaining this behavior and optionally mention that plugin configs (e.g., claude/devin/codex patterns) show all three valid patterns.
🤖 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 `@src-tauri/src/plugin_engine/manifest.rs`:
- Around line 273-367: The file exceeds the ~400 LOC cap because the
#[cfg(test)] block with unit tests was added; extract that entire #[cfg(test)] {
... } block into a sibling test module/file (keep the tests unchanged) and
import the tested symbols (parse_manifest and weekly_candidate) from the current
module so the tests compile; ensure the new test module is annotated with
#[cfg(test)] and uses the same visibility/imports (use
crate::plugin_engine::manifest::{parse_manifest, weekly_candidate}; or
equivalent) so the tests run as before and the runtime module stays under the
size guideline.
In `@src/hooks/app/use-settings-display-actions.ts`:
- Around line 82-87: The catch in handleMenubarMetricChange currently only logs
persistence failures from saveMenubarMetric to console, so surface the failure
to users and fail loudly: in the catch block (inside handleMenubarMetricChange
where saveMenubarMetric is called) call the app toast/error UI (e.g.,
toast.error) with a clear message including the error, keep console.error for
logs, and rethrow the error (or return a rejected promise) so the failure is not
silently swallowed; ensure toast is imported/available and no silent swallowing
remains while preserving setMenubarMetric and scheduleTrayIconUpdate calls.
In `@src/pages/settings.tsx`:
- Around line 493-515: The settings.tsx file has grown too large—extract the
Menubar metric UI into a new React component (e.g., MenubarMetricSelector) that
accepts props: options (MENUBAR_METRIC_OPTIONS), value (menubarMetric) and
onChange (onMenubarMetricChange); move the JSX block that renders the <p>,
container div and map over MENUBAR_METRIC_OPTIONS into that component, preserve
roles/aria attributes and Button props (variant, size, aria-checked) and update
the original settings component to import and render <MenubarMetricSelector
options={MENUBAR_METRIC_OPTIONS} value={menubarMetric}
onChange={onMenubarMetricChange} /> to keep behavior identical while reducing
settings.tsx size.
---
Outside diff comments:
In `@docs/plugins/schema.md`:
- Line 278: Update the example JSON object that currently uses the deprecated
"primary": true flag to the new numeric sorting key: remove the "primary"
property and add "primaryOrder": 0 (or the appropriate numeric rank) to indicate
its priority; target the JSON entry with keys "type": "progress", "label":
"Usage", "scope": "overview" and replace the boolean primary with the numeric
primaryOrder.
---
Nitpick comments:
In `@docs/plugins/schema.md`:
- Around line 127-141: Add a clarifying sentence to the Weekly Metric section:
state that a line with "period": "weekly" may optionally include a primaryOrder
value to also serve as a primary candidate (so a weekly line can be both the
weekly override and count toward primary ordering), and keep the existing rule
that a provider must still define at least one primaryOrder line to appear in
the menubar; reference the JSON keys "lines", "period", and "primaryOrder" when
explaining this behavior and optionally mention that plugin configs (e.g.,
claude/devin/codex patterns) show all three valid patterns.
In `@src/hooks/app/use-settings-display-actions.test.ts`:
- Around line 115-156: Add a failing persistence case for the menubar metric in
the "logs persistence failures" test: have
saveMenubarMetricMock.mockRejectedValueOnce(new Error("menubar metric failed")),
invoke result.current.handleMenubarMetricChange(...) inside the act block (use
the same pattern as handleThemeModeChange/handleDisplayModeChange), and add an
expectation in the waitFor that console.error was called with the matching
message ("Failed to save menubar metric:") and the error; this updates the test
around useSettingsDisplayActions, handleMenubarMetricChange, and
saveMenubarMetricMock to assert menubar metric failure logging.
In `@src/lib/tray-primary-progress.test.ts`:
- Line 92: Update the test expectation to explicitly include the label field so
the shape matches other assertions: change the toEqual assertion that checks
bars (currently comparing to [{ id: "a", fraction: undefined }]) to include
label: undefined as well; locate the assertion referencing bars in
tray-primary-progress.test (the expect(bars).toEqual(...) line) and add label:
undefined to the expected object to match the explicit expectations used
elsewhere.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4da64ba0-166b-4295-b06f-b3c3f0764a0e
📒 Files selected for processing (28)
docs/plugins/schema.mdplugins/claude/plugin.jsonplugins/codex/plugin.jsonplugins/devin/plugin.jsonplugins/kimi/plugin.jsonplugins/opencode-go/plugin.jsonplugins/opencode-go/plugin.test.jsplugins/zai/plugin.jsonsrc-tauri/src/lib.rssrc-tauri/src/plugin_engine/manifest.rssrc/App.test.tsxsrc/App.tsxsrc/components/app/app-content.tsxsrc/hooks/app/use-settings-bootstrap.test.tssrc/hooks/app/use-settings-bootstrap.tssrc/hooks/app/use-settings-display-actions.test.tssrc/hooks/app/use-settings-display-actions.tssrc/hooks/app/use-tray-icon.tssrc/lib/plugin-types.tssrc/lib/settings.test.tssrc/lib/settings.tssrc/lib/tray-primary-progress.test.tssrc/lib/tray-primary-progress.tssrc/lib/tray-tooltip.test.tssrc/lib/tray-tooltip.tssrc/pages/settings.test.tsxsrc/pages/settings.tsxsrc/stores/app-preferences-store.ts
Comment on lines
+82
to
+87
| const handleMenubarMetricChange = useCallback((metric: MenubarMetric) => { | ||
| setMenubarMetric(metric) | ||
| scheduleTrayIconUpdate("settings", 0) | ||
| void saveMenubarMetric(metric).catch((error) => { | ||
| console.error("Failed to save menubar metric:", error) | ||
| }) |
There was a problem hiding this comment.
Surface metric-save failures to users, not just logs.
Line 85 catches persistence failures but only logs to console.error, so users won’t know their preference didn’t save. Please route this through the app’s user-visible error channel too.
As per coding guidelines **/*.{ts,tsx,js,jsx}: Use explicit result types for expected issues instead of throw/try/catch; fail LOUD with throw/console.error + toast.error for unexpected issues.
🤖 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 `@src/hooks/app/use-settings-display-actions.ts` around lines 82 - 87, The
catch in handleMenubarMetricChange currently only logs persistence failures from
saveMenubarMetric to console, so surface the failure to users and fail loudly:
in the catch block (inside handleMenubarMetricChange where saveMenubarMetric is
called) call the app toast/error UI (e.g., toast.error) with a clear message
including the error, keep console.error for logs, and rethrow the error (or
return a rejected promise) so the failure is not silently swallowed; ensure
toast is imported/available and no silent swallowing remains while preserving
setMenubarMetric and scheduleTrayIconUpdate calls.
Author
There was a problem hiding this comment.
Intentionally matching the existing pattern. All six settings-save handlers (theme, display, reset-timer, time-format, icon-style, and this new metric) use console.error only, and there's no toast system anywhere in src. Adding a user-facing toast to just this one handler would be inconsistent, and wiring toast for settings persistence is a separate, cross-cutting change. Left as-is for consistency.
There was a problem hiding this comment.
@michaljuris, that's a fair and well-reasoned call — consistency across all six handlers is more important than applying the toast guideline to just one of them when the infrastructure doesn't exist yet. Acknowledged.
Would you like me to open a follow-up GitHub issue to track adding user-visible error feedback (e.g. toast notifications) across all settings-save handlers as a cross-cutting improvement? That way the guideline can be applied uniformly when the time comes.
✏️ Learnings added
Learnt from: michaljuris
Repo: robinebers/openusage PR: 555
File: src/hooks/app/use-settings-display-actions.ts:82-87
Timestamp: 2026-06-04T18:27:03.808Z
Learning: In `src/hooks/app/use-settings-display-actions.ts` (TypeScript/React, robinebers/openusage), all settings-save handlers (theme, display, reset-timer, time-format, icon-style, menubar-metric) intentionally use `console.error` only in their catch blocks. There is no toast/user-visible error notification system anywhere in `src`. Adding a toast to any individual handler would be inconsistent; wiring toast for settings persistence is a deliberate future cross-cutting change, not something to apply piecemeal.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: robinebers/openusage PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T09:43:35.940Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use explicit result types for expected issues instead of throw/try/catch; fail LOUD with throw/console.error + toast.error for unexpected issues
Author
There was a problem hiding this comment.
The decision to open a follow-up issue should be left to the maintainer.
Comment on lines
+493
to
+515
| <p className="text-sm text-muted-foreground mt-3 mb-2">Metric</p> | ||
| <div className="bg-muted/50 rounded-lg p-1"> | ||
| <div className="flex gap-1" role="radiogroup" aria-label="Menubar metric"> | ||
| {MENUBAR_METRIC_OPTIONS.map((option) => { | ||
| const isActive = option.value === menubarMetric; | ||
| return ( | ||
| <Button | ||
| key={option.value} | ||
| type="button" | ||
| role="radio" | ||
| aria-label={option.label} | ||
| aria-checked={isActive} | ||
| variant={isActive ? "default" : "outline"} | ||
| size="sm" | ||
| className="flex-1" | ||
| onClick={() => onMenubarMetricChange(option.value)} | ||
| > | ||
| {option.label} | ||
| </Button> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split src/pages/settings.tsx; it is well past the ~400 LOC cap.
This file is now ~591 lines, and the new Metric UI adds to an already oversized module. Please extract settings sections (for example, Menubar controls) into focused components.
As per coding guidelines **/*.{js,ts,tsx,jsx,rs,py,java,go,cpp,c,rb}: Keep files under ~400 lines of code; split/refactor as needed.
🤖 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 `@src/pages/settings.tsx` around lines 493 - 515, The settings.tsx file has
grown too large—extract the Menubar metric UI into a new React component (e.g.,
MenubarMetricSelector) that accepts props: options (MENUBAR_METRIC_OPTIONS),
value (menubarMetric) and onChange (onMenubarMetricChange); move the JSX block
that renders the <p>, container div and map over MENUBAR_METRIC_OPTIONS into
that component, preserve roles/aria attributes and Button props (variant, size,
aria-checked) and update the original settings component to import and render
<MenubarMetricSelector options={MENUBAR_METRIC_OPTIONS} value={menubarMetric}
onChange={onMenubarMetricChange} /> to keep behavior identical while reducing
settings.tsx size.
Author
There was a problem hiding this comment.
This file was already ~561 LOC before this PR (now ~590); the new Metric control is one more section consistent with the nine already inline. Extracting only it wouldn't bring the file under ~400, and a holistic split of settings.tsx is out of scope for this feature — better as its own refactor. Left as-is.
There was a problem hiding this comment.
@michaljuris — Fair point. The pre-existing size already violated the guideline, and cherry-picking only the Metric block wouldn't bring the file into compliance. A dedicated refactor PR is the right venue for a holistic split. Would you like me to open a follow-up GitHub issue to track that work?
🧠 Learnings used
Learnt from: CR
Repo: robinebers/openusage PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-02T09:43:35.940Z
Learning: Applies to **/*.{js,ts,tsx,jsx,rs,py,java,go,cpp,c,rb} : Keep files under ~400 lines of code; split/refactor as needed
Author
There was a problem hiding this comment.
The decision to open a follow-up issue should be left to the maintainer.
Owner
|
I wish all of the PRs open would be as thorough as this. We'll review this for the next release. Thank you so much. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a global Metric toggle (Default | Weekly) under Settings → Menubar Icon. When set to Weekly, the menubar icon (all styles) and its tooltip show a provider's weekly usage instead of its default/primary metric. Providers without a weekly line keep showing their primary metric, so the icon never blanks out.
This is a fresh, simplified take on the stalled #466 — addressing the missing screenshots and a code-quality concern with its detection approach.
How it works
plugin.jsonwith"period": "weekly".weeklyCandidatelabel onPluginMeta, mirroring the existingprimaryCandidatesmechanism.meta.weeklyCandidate— no display-label matching. (Add weekly menubar limit preference #466 detected weekly via a/weekly/iregex on label text, which is brittle: labels get renamed/localized — e.g. the recent "Devin weekly quota" rename.)Tooltip
In weekly mode the per-provider tooltip tags lines with their real metric label only when the list is mixed (some providers fell back to their primary). When every shown provider is weekly the tags are redundant and omitted; default-mode tooltips are unchanged.
Type of Change
Testing
bun run build— passes (tsc + vite)bun run test— 64 files, 1107 tests pass (new coverage: settings load/save, weekly selection + fallback, tooltip tagging, the settings control, Rust manifest parsing)cargo test(src-tauri) — 120 tests pass across 3 suites, including the two newmanifest.rstests forperiod/weekly_candidate.Screenshots
Menubar tray icon — before/after (no real change)
The actual menu-bar effect across the three icon styles (Default → Weekly):
Settings → Menubar Icon — the new Metric toggle
Shown at the real 400px panel width, with the sections above (Time Format) and below (App Theme) for context.
Tooltip
Mixed, dark:
Related
Checklist
mainprimaryOrder)docs/plugins/schema.md)Summary by CodeRabbit
New Features
Documentation