Skip to content

Add weekly menubar metric preference#555

Merged
robinebers merged 1 commit into
robinebers:mainfrom
michaljuris:feat/menubar-weekly-metric
Jun 6, 2026
Merged

Add weekly menubar metric preference#555
robinebers merged 1 commit into
robinebers:mainfrom
michaljuris:feat/menubar-weekly-metric

Conversation

@michaljuris
Copy link
Copy Markdown
Contributor

@michaljuris michaljuris commented Jun 4, 2026

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

  • Providers declare their weekly line in plugin.json with "period": "weekly".
  • The Rust manifest loader resolves that into a weeklyCandidate label on PluginMeta, mirroring the existing primaryCandidates mechanism.
  • The tray reads meta.weeklyCandidateno display-label matching. (Add weekly menubar limit preference #466 detected weekly via a /weekly/i regex on label text, which is brittle: labels get renamed/localized — e.g. the recent "Devin weekly quota" rename.)
  • The default is each provider's existing primary metric, so behavior is unchanged until toggled. The left option is labeled Default (not "Session") because most providers' primary isn't a session — e.g. Cursor→Credits, Copilot→Premium, Amp→Free.
  • Marked providers: Claude, Codex, Devin, Kimi, OpenCode Go, Z.ai.

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

  • New feature

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 new manifest.rs tests for period / weekly_candidate.

Screenshots

Menubar tray icon — before/after (no real change)

The actual menu-bar effect across the three icon styles (Default → Weekly):

Icon style Before (Default) After (Weekly)
Provider (icon + %) image image
Bars image image
Donut image image

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.

Default Weekly
Light
Dark

Tooltip

Illustrative render — the text is the exact output of formatTrayTooltip; the bubble styling approximates the native macOS tooltip.

Default Weekly (all-weekly) Weekly (mixed)

Mixed, dark:

Related

Checklist

  • Read CONTRIBUTING.md
  • Targets main
  • No new dependencies
  • Matches existing design language (segmented control like the other Menubar settings; declarative manifest field mirroring primaryOrder)
  • Docs updated (docs/plugins/schema.md)

Summary by CodeRabbit

  • New Features

    • Added "Menubar Metric" setting to toggle between default and weekly metrics for the tray display; selecting weekly updates tray bars and tooltips accordingly.
    • Plugins can mark a weekly progress line so providers with weekly data appear differently when weekly metric is selected.
  • Documentation

    • Updated plugin schema docs to replace the boolean tray marker with a numeric primaryOrder, describe weekly-period support, selection rules, and examples.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges.

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7299dd0b-7bff-40ef-85f7-bb51784703f2

📥 Commits

Reviewing files that changed from the base of the PR and between 90e4dc3 and 1465325.

📒 Files selected for processing (28)
  • docs/plugins/schema.md
  • plugins/claude/plugin.json
  • plugins/codex/plugin.json
  • plugins/devin/plugin.json
  • plugins/kimi/plugin.json
  • plugins/opencode-go/plugin.json
  • plugins/opencode-go/plugin.test.js
  • plugins/zai/plugin.json
  • src-tauri/src/lib.rs
  • src-tauri/src/plugin_engine/manifest.rs
  • src/App.test.tsx
  • src/App.tsx
  • src/components/app/app-content.tsx
  • src/hooks/app/use-settings-bootstrap.test.ts
  • src/hooks/app/use-settings-bootstrap.ts
  • src/hooks/app/use-settings-display-actions.test.ts
  • src/hooks/app/use-settings-display-actions.ts
  • src/hooks/app/use-tray-icon.ts
  • src/lib/plugin-types.ts
  • src/lib/settings.test.ts
  • src/lib/settings.ts
  • src/lib/tray-primary-progress.test.ts
  • src/lib/tray-primary-progress.ts
  • src/lib/tray-tooltip.test.ts
  • src/lib/tray-tooltip.ts
  • src/pages/settings.test.tsx
  • src/pages/settings.tsx
  • src/stores/app-preferences-store.ts
✅ Files skipped from review due to trivial changes (4)
  • plugins/devin/plugin.json
  • plugins/zai/plugin.json
  • plugins/codex/plugin.json
  • plugins/claude/plugin.json
🚧 Files skipped from review as they are similar to previous changes (24)
  • plugins/kimi/plugin.json
  • plugins/opencode-go/plugin.json
  • src/lib/tray-tooltip.test.ts
  • plugins/opencode-go/plugin.test.js
  • src/lib/plugin-types.ts
  • src-tauri/src/lib.rs
  • src/App.test.tsx
  • src/lib/tray-primary-progress.ts
  • src/lib/tray-tooltip.ts
  • src/pages/settings.test.tsx
  • src/stores/app-preferences-store.ts
  • src/lib/tray-primary-progress.test.ts
  • src/App.tsx
  • src/components/app/app-content.tsx
  • docs/plugins/schema.md
  • src/hooks/app/use-settings-bootstrap.ts
  • src/lib/settings.test.ts
  • src/hooks/app/use-settings-display-actions.ts
  • src/hooks/app/use-settings-display-actions.test.ts
  • src/hooks/app/use-settings-bootstrap.test.ts
  • src/lib/settings.ts
  • src/hooks/app/use-tray-icon.ts
  • src-tauri/src/plugin_engine/manifest.rs
  • src/pages/settings.tsx

📝 Walkthrough

Walkthrough

Adds a persisted menubar metric preference ("default" | "weekly"), a period: "weekly" manifest field and primaryOrder docs, backend weekly-candidate resolution, store/bootstrap wiring, settings UI and handlers, and tray/tooltip selection that can prefer weekly progress lines and surface their labels.

Changes

Weekly Menubar Metric Feature

Layer / File(s) Summary
Plugin schema and manifest configuration
docs/plugins/schema.md, plugins/*/plugin.json, plugins/opencode-go/plugin.test.js
Docs replace primary with primaryOrder, document primary-candidate selection, and add period: "weekly" to weekly progress lines; plugin manifests and test expectations updated.
Backend manifest parsing and weekly candidate resolution
src-tauri/src/plugin_engine/manifest.rs, src-tauri/src/lib.rs, src/lib/plugin-types.ts
ManifestLine accepts period; weekly_candidate() returns first weekly progress line label; PluginMeta.weekly_candidate populated for listing plugins.
Settings type definition and persistence
src/lib/settings.ts, src/lib/settings.test.ts
Add MenubarMetric type, storage key, default/options, isMenubarMetric, and loadMenubarMetric/saveMenubarMetric persistence helpers with tests.
App preferences store and bootstrap
src/stores/app-preferences-store.ts, src/hooks/app/use-settings-bootstrap.ts, src/hooks/app/use-settings-bootstrap.test.ts
Store adds menubarMetric and setMenubarMetric; bootstrap loads stored metric with default fallback and error logging; tests added.
Settings page UI and display actions handler
src/pages/settings.tsx, src/pages/settings.test.tsx, src/hooks/app/use-settings-display-actions.ts, src/hooks/app/use-settings-display-actions.test.ts
SettingsPage renders a "Metric" radio-group; handleMenubarMetricChange updates state, schedules tray update, and persists the selection; tests assert UI and persistence calls.
App component wiring and propagation
src/App.tsx, src/components/app/app-content.tsx
Wire menubarMetric and setter through App → AppContent → SettingsPage and pass metric into useTrayIcon.
Tray icon bar selection and rendering
src/hooks/app/use-tray-icon.ts, src/lib/tray-primary-progress.ts, src/lib/tray-primary-progress.test.ts
useTrayIcon derives preferWeekly and passes it to getTrayPrimaryBars; bars now include label and weekly; selection prefers weekly candidate when available; tests updated to assert labels and weekly behavior.
Tooltip formatting with weekly mode support
src/lib/tray-tooltip.ts, src/lib/tray-tooltip.test.ts
formatTrayTooltip accepts weeklyMode and conditionally appends metric labels when mixed weekly/non-weekly bars exist; tests added.
Integration and UI tests
src/App.test.tsx and hook tests
Mocks updated to include load/save menubar metric; new tests verify bootstrap fallback, setting changes re-render tray with weekly fractions, and tooltip label behavior.

Sequence Diagram

sequenceDiagram
  participant User
  participant SettingsPage
  participant DisplayActions
  participant Store
  participant TrayIcon
  participant Backend

  User->>SettingsPage: select "Weekly"
  SettingsPage->>DisplayActions: onMenubarMetricChange("weekly")
  DisplayActions->>Store: setMenubarMetric("weekly")
  DisplayActions->>DisplayActions: saveMenubarMetric("weekly")
  DisplayActions->>TrayIcon: schedule update
  TrayIcon->>Store: read menubarMetric
  TrayIcon->>Backend: request PluginMeta (uses weekly_candidate)
  TrayIcon->>TrayIcon: getTrayPrimaryBars(preferWeekly=true)
  TrayIcon->>TrayIcon: formatTrayTooltip(weeklyMode=true)
  TrayIcon->>User: render tray bars and tooltip
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • davidarny
  • validatedev
  • robinebers
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.48% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add weekly menubar metric preference' directly describes the main feature addition—a new metric preference option (Default or Weekly) for the menubar. It aligns with the core objective of allowing users to toggle between default and weekly metrics in settings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions github-actions Bot added rust Pull requests that update rust code core plugin docs labels Jun 4, 2026
@michaljuris michaljuris force-pushed the feat/menubar-weekly-metric branch 2 times, most recently from 4e03de7 to b028041 Compare June 4, 2026 12:02
@michaljuris michaljuris force-pushed the feat/menubar-weekly-metric branch from b028041 to 90e4dc3 Compare June 4, 2026 17:40
@michaljuris michaljuris marked this pull request as ready for review June 4, 2026 17:45
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 28 files

Re-trigger cubic

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 win

Update example to use new primaryOrder schema.

This example still uses the deprecated "primary": true boolean syntax. It should be updated to match the new primaryOrder numeric 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 value

Consider explicitly asserting label: undefined for consistency.

Other updated test expectations in this file now explicitly include the label field (e.g., lines 166, 203, 241, 286). For consistency and clarity, this test should also assert label: undefined to 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 value

Consider 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 saveMenubarMetricMock and asserting the corresponding console.error call.

🧪 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 win

Consider 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 have primaryOrder. Looking at the plugin configs:

  • claude/plugin.json line 15: Weekly line has primaryOrder: 2 (serves as both weekly candidate and secondary primary)
  • devin/plugin.json line 10: Weekly quota has primaryOrder: 1 (weekly IS the primary)
  • codex/plugin.json line 15: Weekly line has NO primaryOrder (weekly-only, not a primary candidate)

All three patterns are valid and work correctly. A sentence clarifying "A weekly line may optionally include primaryOrder to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5b337 and 90e4dc3.

📒 Files selected for processing (28)
  • docs/plugins/schema.md
  • plugins/claude/plugin.json
  • plugins/codex/plugin.json
  • plugins/devin/plugin.json
  • plugins/kimi/plugin.json
  • plugins/opencode-go/plugin.json
  • plugins/opencode-go/plugin.test.js
  • plugins/zai/plugin.json
  • src-tauri/src/lib.rs
  • src-tauri/src/plugin_engine/manifest.rs
  • src/App.test.tsx
  • src/App.tsx
  • src/components/app/app-content.tsx
  • src/hooks/app/use-settings-bootstrap.test.ts
  • src/hooks/app/use-settings-bootstrap.ts
  • src/hooks/app/use-settings-display-actions.test.ts
  • src/hooks/app/use-settings-display-actions.ts
  • src/hooks/app/use-tray-icon.ts
  • src/lib/plugin-types.ts
  • src/lib/settings.test.ts
  • src/lib/settings.ts
  • src/lib/tray-primary-progress.test.ts
  • src/lib/tray-primary-progress.ts
  • src/lib/tray-tooltip.test.ts
  • src/lib/tray-tooltip.ts
  • src/pages/settings.test.tsx
  • src/pages/settings.tsx
  • src/stores/app-preferences-store.ts

Comment thread src-tauri/src/plugin_engine/manifest.rs
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)
})
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The decision to open a follow-up issue should be left to the maintainer.

Comment thread src/pages/settings.tsx
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>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 4, 2026

Choose a reason for hiding this comment

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

🛠️ 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor Author

@michaljuris michaljuris Jun 4, 2026

Choose a reason for hiding this comment

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

The decision to open a follow-up issue should be left to the maintainer.

@robinebers
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Owner

@robinebers robinebers left a comment

Choose a reason for hiding this comment

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

Hey! This is Rob's AI reviewer. Thanks for the contribution.

This is a clean feature overall. The settings flow, tray fallback behavior, and tests look solid. Two small things to fix before merge:

  1. Update the plugin schema examples - docs/plugins/schema.md still shows primary: true in the JSON examples, but the real field is primaryOrder. Since this PR is formalizing weekly metrics, the examples should match the current schema.

  2. Update from main and rerun checks - the PR is currently behind main. Please bring it up to date and rerun the normal checks before merge.

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 michaljuris force-pushed the feat/menubar-weekly-metric branch from 90e4dc3 to 1465325 Compare June 6, 2026 09:14
@michaljuris
Copy link
Copy Markdown
Contributor Author

Thanks Rob — both addressed:

  1. Schema examples — corrected the two remaining primary: true blocks (the full-manifest example near the top and the "Minimal Example" near the bottom of docs/plugins/schema.md) to primaryOrder: 1. My earlier commit only updated the "Primary Progress" section and the field table, so those two example blocks slipped through — good catch. The whole doc is consistent with primaryOrder now.

  2. Up to date — rebased onto latest main (now 0.6.27), 0 commits behind. Reran the checks on the rebased branch: bun run build ✓ and bun run test ✓ (1109 passing). The Rust crate compiles in CI.

Re-requested review when you have a moment 🙏

@michaljuris michaljuris requested a review from robinebers June 6, 2026 09:18
Copy link
Copy Markdown
Owner

@robinebers robinebers left a comment

Choose a reason for hiding this comment

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

Looks good. The earlier schema/docs concern is addressed, and the full local test/build pass is clean.

@robinebers robinebers merged commit 009b409 into robinebers:main Jun 6, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core docs plugin rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants