feat(desktop): deliver rate-limit notifications natively in the Tauri app - #1706
Closed
jzila wants to merge 3 commits into
Closed
feat(desktop): deliver rate-limit notifications natively in the Tauri app#1706jzila wants to merge 3 commits into
jzila wants to merge 3 commits into
Conversation
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 10, 2026 22:17
991fcd5 to
20751da
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 10, 2026 23:56
20751da to
075cc47
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
2 times, most recently
from
September 11, 2026 00:29
04db186 to
c3b00b3
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 00:58
c3b00b3 to
5ff3f3d
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 01:36
5ff3f3d to
d1ad8b3
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 02:21
d1ad8b3 to
8fbef4f
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
2 times, most recently
from
September 11, 2026 03:08
8b74c2a to
78d11f7
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 03:49
78d11f7 to
3b7a2b7
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 04:45
3b7a2b7 to
5072820
Compare
roborev: Combined Review (
|
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 05:14
5072820 to
e0bca43
Compare
roborev: Combined Review (
|
jzila
marked this pull request as ready for review
September 11, 2026 05:50
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 06:11
e0bca43 to
cf591ba
Compare
roborev: Combined Review (
|
jzila
marked this pull request as draft
September 11, 2026 06:32
…e page Codex CLI writes a `rate_limits` object into its `token_count` events (up to two windows, a short `primary` window and a longer `secondary` one, e.g. 5 hours and 7 days) alongside the plan type and credit balance. agentsview now extracts that object during Codex parsing, persists each observation, and surfaces it on the Usage page as a "Rate limits" section: one card per window showing used percent, time to reset, plan type, credit balance, and a small history chart of used percent over the selected date range. Schema and identity Observations are stored in a new `rate_limit_snapshots` table (SQLite only, alongside cursor_usage_events and the Codex incremental-import tables -- out of scope for the SQLite/PostgreSQL/ DuckDB parity rule). The table is vendor-keyed (`vendor`, `'codex'` for every row today) from the start rather than Codex-specific, because a second PR building on this one adds a Claude rate-limit poller that needs the same table: shipping the vendor-neutral shape now means that PR only adds rows, instead of carrying a migration that renames the table and backfills a `vendor` column for a table that had not even shipped to users yet. `account_id`, `account_label`, `scope_label`, and `details` are reserved the same way for a vendor whose rate-limit source has that shape, and stay empty on every Codex row. Codex rollouts carry no stable per-account identifier -- no account id, user id, email, or org field appears in session_meta or token_count payloads (see docs/internal/session-format-sources.md) -- so a Codex snapshot's identity is (machine, limit_id, plan_type, window_kind) rather than an account. Rows are written with INSERT OR IGNORE against a unique dedup_key (source session id + observed timestamp + limit id + window kind), never a delete-then-reinsert, so both a full parse and an incremental parse converge on the same rows without duplicating or losing history. session_id is nullable (ON DELETE SET NULL) so a row survives its source session being deleted. resets_at is also nullable end to end -- the parser type, the table, and the API response all preserve a genuinely unknown reset time as null rather than flattening it to 0, which previously would have shown as a bogus countdown to the unix epoch. Because this backfills history from existing rollout files, the parser data version is bumped (to 108) so an archive written by an older binary gets a full reparse and picks up the rate_limits history its rollout files already contained, instead of silently staying empty until the next observation. The "current" query (LatestRateLimitSnapshots) resolves "latest" per (vendor, machine, account_id, limit_id, plan_type) bucket, one level above window_kind, and returns every window belonging to that bucket's single newest observation, rather than ranking each window_kind independently: the latter let a window that stopped being reported (e.g. a session moving from primary+secondary windows to primary-only) keep showing its last-known row indefinitely. The winning observation is further disambiguated by a persisted observation_key column, stored once at insert time from the then-current session_id and independent of the nullable session_id foreign key: two different sessions on one machine and account can legitimately report the same limit/plan at the exact same instant (Codex's rate limit is account-wide), and using session_id itself for this at query time broke either when two such sessions were both deleted (their now-NULL session_ids would compare equal again) or, if that NULL case was special-cased per row instead, when one deleted session's own primary+secondary windows could no longer be told apart from each other. A key fixed at write time and never recomputed avoids both failure modes. The history endpoint downsamples a wide date range instead of returning every matching observation: `max_points` (default 500) caps the response, dividing the range into that many equal-width time buckets and keeping only the most recently observed row per bucket, so a long-lived window's full history does not grow the response (or RateLimitHistoryChart's point count) unboundedly. The chart itself requests a smaller point budget sized for its own sparkline-sized rendering. API and frontend New routes `GET /api/v1/rate-limits/current` and `.../history` accept `vendor`, `account_id`, and `machine` filters (`agent` is kept as a deprecated alias for `vendor`, matching the shared session filter's comma-separated selection semantics via `RateLimitAgentMatchesVendor`); `history` additionally accepts `limit_id`, `window`, `since`, `until`, and `max_points`. PostgreSQL and DuckDB implement the read side as no-ops, so the section is simply hidden when either backend is the active read store. The frontend groups cards by vendor and then by account; since Codex reports no account identity, its account group is keyed by machine instead, so two machines syncing the same account still render as two groups. A window with no known reset time shows "Reset time unknown" instead of a countdown. The history chart now plots observed time on a real time scale (it previously used a categorical point scale that spaced every observation evenly by index, which visually flattened bursts and gaps in the actual observation cadence). Robustness fixes folded in from post-implementation review - A full resync now copies existing rate_limit_snapshots rows into the replacement archive, the same way model pricing is copied, so previously observed windows survive the swap instead of going empty until the next observation. This copy runs after orphaned sessions are restored, not before: session_id is a foreign key, and a snapshot belonging to an orphaned session would otherwise violate it during the copy (INSERT OR IGNORE does not suppress a foreign-key violation the way it suppresses a duplicate), silently losing every row in the table rather than just that session's. The copy also NULLs session_id for any row whose session still does not exist in the destination even after orphan restoration -- one superseded by a reparse under a different id, or one excluded as parser-excluded -- rather than copying that id unchanged and hitting the same constraint; dedup_key and observation_key are preserved from the source unchanged either way. This history cannot be reconstructed once lost, so a failed copy aborts the swap instead of merely warning. - A rate_limits payload missing limit_id is skipped rather than failing the whole write -- since this write is normally part of a larger session write, the old behavior could fail an entire session's ingestion over one malformed rate-limit entry. - The machine and agent/vendor filters use the same comma-separated IN/contains semantics as the rest of the shared session filters, and rate-limit history queries parse and normalize since/until as RFC3339Nano rather than comparing raw request strings against the stored UTC column. - The incremental-parse cursor seed (a prefix scan that reconstructs cursor state without ever returning a session result) no longer accumulates rate-limit observations while scanning: they were never read out of that path, so collecting them grew memory with the whole scanned prefix on every incremental-parse cache miss instead of staying bounded. docs/agents/storage.md and docs/token-usage.md are updated to describe the new table and endpoints.
Adds a Notifications settings panel with a Rate limits card so a user can get a browser notification when a vendor's rate-limit window is about to run out, instead of having to keep the Usage page open and watch the bars. The card has a single slider, 0-25% remaining, that sets the threshold at which a source first notifies, plus a separate checkbox for a second "exhausted" stage (0% remaining) that fires independently of the slider. Sources are generalized across vendor + accountId (not just Codex), each with its own per-source mute so a noisy account can be silenced without losing alerts on the rest. The browser's notification permission is requested only when the user turns the feature on, never on page load, and a denied/blocked permission surfaces in the panel instead of failing silently. State lives in localStorage: settings (threshold, exhausted toggle, mutes) under one key, and per-window notified/armed state under another, keyed by vendor:account:limitId:windowKind so distinct windows on the same account don't collide or share one arming entry (planType is a label, not part of the key -- see below). A storage event listener keeps multiple open tabs in sync so the same source doesn't fire twice from two tabs and a mute toggled in one tab takes effect in the others immediately. A fired stage re-arms in two ways: normally when the API reports a new, later resetsAt for that window (a fresh cycle), and, for a window whose resetsAt is unknown (null, which the API now reports instead of the old 0-sentinel), when the window's usedPercent recovers back above the threshold — since there's no reset timestamp to key a rearm off, recovery is treated as the signal that the prior exhaustion cycle ended. All the resetsAt handling in rateLimitAlerts.ts now checks for null rather than 0, and a resetsAt of exactly 0 is no longer treated as "known and elapsed." Notification delivery goes through a small notifier abstraction rather than calling the browser Notification API directly from the runner, so a later desktop build can swap in a native notifier without touching the threshold/arming logic. Paraglide message keys were added for every new label, slider, checkbox, and notification string across all six locales (en, fr, ja, ko, zh-CN, zh-TW), keeping identical key sets per project.inlang/settings.json and formatted with vp fmt. Rebased onto the squashed codex-rate-limits commit, which kept moving throughout this rebase (7772c79 -> 40ba274 amended ResetsAt from a plain `int64` to `*int64` with `json:",omitempty"`, see below; several further db-layer/parser/test-only follow-ups; a RateLimitCard/ rateLimitFormat epoch-zero consistency fix; and a final change dropping planType from the card's own RateLimitCardIdentity/historyKey and deleting matchesIdentity, none of which this branch's own window key -- which keeps planType deliberately, for its own, separate reason -- reads or reimplements; a further change gating rate-limit extraction to the codex agent, dropping plan_type from LatestRateLimitSnapshots' partitioning (labels now resolved as latest non-empty) and from RateLimitsSection's own snapshotKey -- again, none of it read or reimplemented by this branch's own window key; a further backend-only change making read-only archives return no rows for a table that doesn't exist there, deleting a session's own rate-limit snapshots on a full session replace, and a new InsertRateLimitSnapshotsReplacingSession export, none of it touching the API/frontend surface; a final change scoping the account_id filter to vendors that actually have accounts (never excluding Codex's account-less rows) and threading an orphaned-session-ids slice through CopyRateLimitSnapshotsFrom, plus two regenerated client models with only a doc-comment wording change; final tip, before codex-rate-limits was itself finally rebased onto current upstream/main -- pulling in a large amount of unrelated upstream history (new raw-sync/parser-provider work, dependency bumps) with no further rate-limit API or export changes -- e0a44e8; then 54381f9; a final internal fix to the table-existence probe in internal/db/db.go and rate_limit_snapshots.go, no exports or frontend surface; final tip 46ee3b2): adapted to the CodexRateLimit* -> RateLimit* rename, the vendor/accountId generalization of the store and types, the removed window-kind badge on RateLimitCard, and history's Until becoming exclusive. Squashed from 12 commits on the prior usage-threshold-notifications branch (backed up as notify-pre-squash) for a clean single-commit history. Local roborev review of the commit rebased onto 7772c79 flagged four items; three were fixed directly here: - parseNotifiedStages() dropped a stage's recoveredAt marker on every parse, even though it round-trips fine through JSON.stringify on write -- a page reload or a cross-tab storage-event hydrate erased the unknown-reset recovery marker evaluateRateLimitWindows depends on, re-suppressing a stage that had already recovered. - An unknown-reset window's exhausted stage kept blocking a fresh threshold notification forever once fired, even after it recorded a recovery (usage dropping back below 100%) -- only a full re-climb back to fully used, which re-arms exhausted itself, used to clear the block. Recovered-but-not-re-armed exhausted no longer suppresses an independent, lower threshold crossing. A follow-up roborev-ci pass then caught that this first version over-corrected: exhausted recovering (100% -> 90%, say) let threshold fire immediately even when usedPercent never actually dropped back below the threshold boundary in between. Fixed by silently recording a threshold-crossing suppressed by exhausted (no notification, but a real stage entry) so it gets its own recovery/re-arm cycle: threshold now only fires again once usedPercent has genuinely dropped below the boundary and been re-crossed (100% -> 90% notifies nothing; 100% -> 70% -> 85% fires). - handleStorageEvent() re-hydrated enabled/mutes/notified-state from another tab's write but never refreshed permission, so a tab open before the user granted permission in another tab stayed stuck reading its own stale permission forever, silently skipping every check() with no reload or toggle to recover. The fourth -- the backend flattening an unknown resetsAt to 0 instead of omitting it, defeating this branch's null-keyed recovery/rearm logic -- was real against 7772c79 but got fixed upstream, in codex-rate-limits itself, by the time this branch was pushed: 40ba274 changed service.RateLimitWindow.ResetsAt from `int64` to `*int64` with `json:",omitempty"`, the generated TS model's `resetsAt` from required to optional, and RateLimitCard.svelte to treat a missing/non-positive value as unknown. Rebasing onto 40ba274 (see above) picks that up for free; no additional change was needed on this branch's side since rateLimitAlerts.ts's adapter already treated anything that wasn't a finite `number` (including `undefined`, which is what a client now sees for an omitted field) as null. Three further review passes (two local roborev, one roborev-ci on the pushed PR) all flagged the same cross-tab race: two tabs reacting to the same SSE event can each evaluate against their own in-memory notified-map before either observes the other's `storage` event -- which only fires after a write, and is itself async -- so both could decide to notify and persist overlapping writes. This went through two fix attempts before landing: first a bare re-hydrate from localStorage right before evaluating (narrowed, didn't close, the race); then a `withCrossTabLock` wrapping the whole read-hydrate/evaluate/persist/notify sequence, using the Web Locks API where available and a short-lived localStorage lease otherwise -- but roborev-ci correctly caught that the lease fallback wasn't actually exclusive either (localStorage has no compare-and-swap, so two tabs can each observe "unclaimed," each write their own token, and each proceed before observing the other's write). frontend/src/lib/utils/crossTabLock.ts's `withCrossTabLock` now uses only the Web Locks API (true, queued mutual exclusion -- every caller either runs or waits, never both at once) with no fallback lock implementation: it's available in every browser and webview this app targets, including WKWebView for the Tauri desktop build, so a hand-rolled substitute that can't match its guarantee isn't worth the false confidence. Where `navigator.locks` is genuinely unavailable, the check still runs (never silently stops delivering notifications), just without cross-tab coordination -- the same single-tab-only behavior this feature had before cross-tab sync existed -- logging once so an unexpectedly old environment is at least visible. Covered by crossTabLock.test.ts (lock present serializes through `navigator.locks.request`; lock absent still delivers). roborev-ci also flagged that evaluateRateLimitWindows' already-reset guard only skipped a window whose resetsAt was both non-null and strictly positive, so a resetsAt of exactly 0 (the unix epoch -- a real, always-elapsed timestamp, not a stand-in for unknown, now that resetsAt is properly nullable end to end per the codex fix above) fell through as if it were an unknown reset instead of an expired one. Dropped the positivity check: any non-null resetsAt that's already passed now skips the window, 0 included. A follow-up roborev-ci pass then caught a second problem in the same area: the rearm check compared resetsAt with `!==`, treating ANY change as a fresh cycle -- including a known->null transition (a vendor observation that transiently omits resetsAt cleared every stage and re-fired the same alert even though usage never actually reset) and a known->known transition to an equal or older timestamp (a stale/ out-of-order observation, also not a real new cycle). Now only a demonstrably later known resetsAt -- both the prior and new values non-null, and the new one strictly greater -- re-arms; every other transition, including either direction between null and known, preserves stage state instead of clearing it (the null-side recovery-based rearm already in place is unaffected, since it keys off the window's current resetsAt rather than the transition itself). A later roborev-ci pass caught a different, longer-standing problem with the same key: `RateLimitWindowInfo.key` had included `planType` since early in this branch's own history (added at a time the API returned a separate row per plan on the same limit/window during a plan transition), but the API's "current" query now resolves one row per (vendor, account, limit, window) with planType attached as a label rather than used to partition rows -- so a plan-label change on the same ongoing window (no actual reset) produced a brand-new key and re-notified for a window that had already fired. Dropped planType from the key (back to its original 4-segment shape, `${vendor}:${account}:${limitId}:${windowKind}`). The same passes also raised one item left as-is because it's dormant under the only vendor actually wired up today (Codex, whose rows never carry an accountId per service.RateLimitWindow's own doc comments): `RateLimitWindowInfo.key` derives `account` from accountId when present, dropping machine -- unlike the API's own vendor+machine+accountId partitioning -- so two machines genuinely sharing one vendor account would collide on one arming entry. Fixing it means widening the key everywhere (window key, notified-map, threshold overrides), which touches every test file that hardcodes a key string; deferred until a multi-machine, account-keyed vendor actually exists to design and test against. A subsequent judge review (kata kemc, against the published PR) found the out-of-order-resetsAt fix above was still incomplete: the evaluator wrote the observed resetsAt straight into the persisted notified-state entry even on a poll that didn't rearm, so a stale, older observation arriving after a newer one (fetches carry no ordering guarantee) still regressed what was remembered -- resets 5000 -> 4000 -> 5000 at a constant usedPercent/threshold produced notification counts 1 -> 0 -> 1 instead of 1 -> 0 -> 0. `latestKnownResetsAt` now tracks the latest known cycle boundary this window has ever reported (unknown loses to any known value, and between two known values the larger wins) and that's what gets persisted, so a stale observation can no longer make a later, genuinely fresh one look like a repeat of an already-notified cycle. Folding that fix in surfaced a second bug in the recovery path it touches: a recoveredAt marker recorded while resetsAt was null (the unknown-reset rearm signal) was only ever consumed while resetsAt was still null, so a window that later learned a real resetsAt stayed suppressed forever even after recovering and re-crossing. Recording a new marker is still gated on resetsAt being null (a known resetsAt has its own rearm signal and must not rearm on a mere dip -- see the existing test for that), but consuming an already-recorded marker now happens regardless of the current resetsAt's null/known state. The same review flagged that the notification label (vendor, account, window duration) dropped the vendor-reported limit name, so two same-duration windows on one account read identically ("Codex · machine-a (pro) · 7d") in a notification title even though the Usage page's own cards distinguish them by name. The runner now builds the notification label the same way RateLimitCard builds its header -- reusing the `rate_limits_window_weekly` / `rate_limits_window_generic` / `rate_limits_card_header` Paraglide messages -- so a notification reads e.g. "Codex · machine-a (pro) · Weekly limit [Spark]". It also asked for the generated `ServiceRateLimitWindow` contract in place of the hand-maintained `CurrentRateLimitApiRow` duplicate (now a type alias to it) and its missing-vendor-defaults-to-codex fallback, which the generated contract's now-required `vendor` field makes unnecessary -- a row with an empty vendor is simply rejected, the same way an empty window kind already was. That also fixed the concrete drift the review pointed at: `details` is a JSON-encoded string on the wire per the generated type, not an object, so `detailsExhaustedFlag` now parses it defensively instead of assuming an already-parsed shape it would never actually see. Finally, the review asked to drop this branch's two localStorage migrations as dead weight on a feature that never shipped: the pre-stage notified-map shape (from before the exhausted stage existed) and the plan-bearing 5-segment key shape `legacyWindowKey`/ `RateLimitAlertSettingsStore.reconcileLegacyKeys` migrated onto the 4-segment shape above. Both are gone; stored state is read in its one current shape only. rateLimitAlerts.ts and its test suite were also trimmed for comment/test bloat the review called out (736 lines with 361 comment-only; 1,243 test lines) -- consolidated into table-driven scenarios per invariant rather than one test per review narrative, while keeping the two-stage/reset/recovery behavior itself covered. A local roborev pass (against the rebase onto codex-rate-limits' 7ce17bb, single commit ad521b0d) found two further issues in the out-of-order-resetsAt handling this branch had just tightened. First, `latestKnownResetsAt` only protected the value PERSISTED as resetsAt -- a fresh cycle that rearmed but didn't yet cross anything still deleted its notified-map entry outright when nothing fired, discarding the newly-known (later) resetsAt instead of remembering it with empty stages. That let a later, stale observation reporting an OLDER resetsAt find no entry to compare against, read as the first-ever observation of a "new" window, and re-fire the alert the real (already-remembered) cycle had already sent. `evaluateRateLimitWindows` now persists `{ resetsAt: nextResetsAt, stages: {} }` on a rearmed poll that doesn't cross, rather than deleting, whenever there's an actual boundary to remember. Second, even with that boundary remembered, a stale observation's usedPercent was still evaluated against it -- a window whose latest known resetsAt is 6000 but that also (out of order) reports a 5000 observation would treat that 5000 packet's usage as a fresh crossing on the still-open 6000 cycle. Stale observations (a known resetsAt strictly older than the remembered one) are now skipped entirely -- no evaluation, no state change -- rather than merely "not treated as a fresh cycle." One sequence test covers the concrete repro: (reset=5000, used=90) fires; (reset=6000, used=10) doesn't cross but must still be remembered; (reset=5000, used=90) is now skipped as stale instead of re-firing. The same pass flagged that `hydrate()` unconditionally re-reads both storage keys, defeating `persistSettings()`/`persistNotified()`'s own best-effort fallback for a failed write (storage full): a notification that fires and updates `notifiedMap` in memory, but fails to persist, was silently reverted by the very next `hydrate()` -- concretely, the poll runner's own pre-evaluation hydrate() inside `withCrossTabLock`, which would then re-fire the same alert; a failed settings write could likewise undo a mute or a disable the same way. `RateLimitAlertSettingsStore` now tracks a per-key failed-write flag (`settingsPersistFailed`, `notifiedPersistFailed`), set in `persistSettings()`/`persistNotified()`'s catch blocks and cleared on the next successful write to that key; `hydrate()` skips re-reading a key while its flag is set, so already-correct in-memory state survives until persistence actually recovers. Covered by a test using a storage stub whose `setItem` throws only for the notified key. The same local roborev pass caught two more issues in the same area. First, unknown-reset recovery was gated purely on `nextResetsAt === null` -- but `latestKnownTimestamp` means a remembered resetsAt only ever loses to a LATER known value, never to null, so a window that had EVER reported a known resetsAt could never again record a recovery once its observations went permanently unknown, even long after that remembered boundary had actually elapsed with no later known value ever arriving to naturally rearm it. Recovery-based rearming is now also allowed once the remembered boundary itself has passed (`nextResetsAt * 1000 <= now`), not only when it was never known to begin with. Second, resetsAt alone can't catch every out-of-order response: two observations can report the identical (including unknown) resetsAt while still describing different points in time, e.g. two racing tabs' polls resolving in reverse order. `RateLimitWindowInfo` and the adapter now carry `observedAtMs` (parsed from the API row's `observedAt`), `NotifiedWindowState` remembers the latest one evaluated (`lastObservedAtMs`, optional so older stored state and fixtures without it still parse as "unknown"), and an observation whose `observedAtMs` is older than what's remembered is skipped the same way a stale resetsAt is -- rejected before it can be misread as a recovery dip. `latestKnownResetsAt` was renamed `latestKnownTimestamp` and now backs both fields, since the "unknown loses, later known wins" rule is identical for each. Two sequence tests cover the concrete regressions. A further local roborev pass on the amended commit found two more issues. First, `evaluateRateLimitWindows` marks a stage as fired in the notified-state map it returns regardless of whether delivery actually succeeds; the runner persisted that map before attempting delivery, so a `new Notification(...)` construction failure (some browsers throw off a user-gesture requirement or in restricted contexts) silently and permanently consumed the alert -- every later poll, and every other tab, then saw that stage as already notified. `fireNotification()` now returns whether it actually constructed a Notification, and `check()` undoes the "notified" mark for any stage whose delivery failed before persisting, so it stays eligible to fire on a later poll. Covered by a runner test (a new file, since none existed for this store yet) using a fake Notification that throws on the first check() and succeeds on the second, confirming the alert is neither dropped nor duplicated. Second, the notification label's window-duration fallback for a window with no reported windowMinutes was a generic "— limit" placeholder -- identical for a primary and a secondary window sharing a limit name, unlike RateLimitCard.svelte's own fallback (added by the codex-rate-limits rebase this branch picked up), which uses the window kind ("Session limit" for primary, "Weekly limit" for secondary) to keep them distinguishable. Extracted that formatter into rateLimitFormat.ts's new `windowLabel()` (duration-aware, windowKind-fallback-aware) and pointed both RateLimitCard.svelte and the notification runner at the one implementation instead of two copies that had already drifted. A final local roborev pass caught that the delivery-failure rollback above was incomplete for the specific case it was added for: a window first observed already exhausted marks BOTH exhausted (queued, notified) and threshold (silently, never queued -- see evaluateRateLimitWindows' doc comment) as fired in the same evaluation. Rolling back only the `toSend.stage` on a failed exhausted delivery left the silently-marked threshold stage fired despite the user never having seen either notification, permanently suppressing a later poll that's simply no longer exhausted but still above the threshold boundary. `NotificationToSend` now carries an optional `silentlyAlsoMarked` list naming any other stage the same evaluation marked as a side effect of this one firing, and `check()`'s rollback undoes those too on a failed delivery. Covered by a runner test: exhausted delivery fails at 100%, then a later poll at 95% (still above a 10%/90%-boundary threshold, no longer exhausted) must still notify.
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 07:19
cf591ba to
61041d1
Compare
… app The desktop shell's webview had no native notification permission delegate wired up: the browser Notification API's requestPermission() silently resolved "denied" with no prompt ever shown, so desktop users never got the rate-limit alerts added by usage-threshold-notifications. Adds the `tauri-plugin-notification` crate, registers it in desktop/src-tauri/src/lib.rs, and grants `notification:default` in desktop/src-tauri/capabilities/default.json. The plugin's own init script monkey-patches window.Notification to route through the native OS notifier, and injects window.__TAURI__.notification with its own isPermissionGranted/requestPermission/sendNotification API. On the frontend, frontend/src/lib/utils/notifier.ts adds a `Notifier` abstraction with two implementations: `BrowserNotifier`, wrapping the existing web `Notification` API, and `DesktopNotifier`, calling the plugin through `window.__TAURI__`. Which one is used is decided purely by feature-detecting `window.__TAURI__` (isDesktopShell()) at call time, never by user-agent sniffing, so a webview outside Tauri or a plain browser tab always falls back to BrowserNotifier. rateLimitAlertRunner's fireNotification() and rateLimitAlertSettings' enable()/refreshPermission() now go through this abstraction instead of calling Notification directly. DesktopNotifier.readPermission() wraps the plugin's isPermissionGranted() in a try/catch: a rejected call (a transient IPC round-trip failure) resolves to "default" instead of propagating, since the method is documented as never rejecting and its caller only attaches a bare .then(). rateLimitAlertRunner.check() now calls the non-prompting refreshPermission() on every poll tick while alerts are enabled but permission isn't "granted", so a transient read failure self-heals on a later tick instead of leaving an already-enabled setup stuck. tauri-plugin-notification's Windows implementation reports permission as "denied" rather than "default" before the user has ever been asked, so the original enable() logic -- which only called requestPermission() when the read came back "default" -- could never prompt a Windows user at all; the toggle just flipped back off. Fixed by branching the request gate on notifier.kind: the desktop path now requests whenever permission isn't already "granted" (still only from the enable() toggle's own user gesture, never on load or on refreshPermission()) and reverts the toggle only if that request itself comes back denied, while the browser path keeps its narrower "default"-only gate. Settings copy (RateLimitAlertSettings.svelte) branches on notifier.kind so the permission status line says "desktop" rather than "browser" while running in the shell, using added *_desktop message keys; the shared default-permission copy was rewritten to be platform-neutral (a French string that said "the browser will ask you" was the one holdout, fixed per a roborev finding). tauri-plugin-notification 2.4.0 has no click/action callback on any desktop platform, only mobile, so the desktop notifier intentionally never wires up onClick rather than fake a callback that would never fire. Rebased many times as usage-threshold-notifications itself moved (codex-rate-limits' own resetsAt-nullability fix; its local roborev fixes; several further rounds of codex-rate-limits' own db-layer/parser/ test-only follow-ups and a final RateLimitCard/rateLimitFormat epoch-zero consistency fix; and several rounds of usage-threshold-notifications' own roborev-ci fixes -- the resetsAt-0-is-not-unknown expiry fix, a first cross-tab lock attempt, and the corrected Web-Locks-only version below); final base is 6fdafb9. Squashed from 4 commits on the prior desktop-notifications branch (backed up as desktop-pre-squash) for a clean single-commit history. usage-threshold-notifications' latest revisions replaced its earlier, partial cross-tab-race mitigations with a real lock: rateLimitAlertRunner .check() now wraps the whole hydrate/reconcile/evaluate/persist/notify sequence in `withCrossTabLock` (frontend/src/lib/utils/crossTabLock.ts). The first attempt paired the Web Locks API with a localStorage-lease fallback for browsers without it; roborev-ci correctly caught that the lease wasn't actually exclusive (no compare-and-swap on localStorage, so two tabs can each see "unclaimed" and both proceed), so the fallback was removed rather than hardened -- Web Locks is available in every browser and webview this app targets, including WKWebView for this desktop build, so `withCrossTabLock` now uses only that, running uncoordinated (logging once) on the rare environment without it rather than pretending a fallback lock is safe. This branch's existing rateLimitAlertRunner.test.ts mock of the settings store predates the original re-hydrate call and didn't define `hydrate`, which broke three existing tests after the first rebase that introduced it; added `hydrate: vi.fn()` to the mock and two tests covering the call directly (that it fires between the fetch and reconcile/evaluate, and that a notified-state write another tab makes during the fetch is honored instead of double-firing). Both subsequent `withCrossTabLock` revisions needed only import-conflict resolutions (this branch's own `getNotifier` import alongside it) -- check()'s body, and this branch's test file and mock, were otherwise unaffected by the lock's internals, since `withCrossTabLock` runs for real (unmocked) against jsdom's real `navigator`/localStorage in these tests. roborev-ci on this branch's own head separately raised a High: nothing guaranteed tauri-plugin-notification's guest JS was actually injected onto `window.__TAURI__.notification` the way the original implementation assumed, and no running desktop build had ever been observed to confirm it (an earlier `tauri dev` verification attempt hung, so that assumption was never actually checked end to end). Replaced the whole approach: `notifier.ts` now detects the desktop shell via `window.__TAURI_INTERNALS__` -- the low-level IPC bridge Tauri v2 unconditionally injects into every webview, unlike the legacy `window.__TAURI__` global that's only present when tauri.conf.json's `app.withGlobalTauri` is set -- and DesktopNotifier talks to notifications through a dynamic `import("@tauri-apps/plugin-notification")` (added to frontend/package.json pinned at 2.4.0, matching desktop/src-tauri/Cargo.lock's resolved Rust crate version exactly) instead of reading a shell-injected global at all. The dynamic import keeps the plugin out of the browser bundle's critical path (confirmed in the build output: `@tauri-apps/plugin-notification`'s code lands in its own chunk, not the main bundle, and `npm run build` passes with no Tauri present); it's cached after the first load so repeated notifier calls within one desktop session share one module instance. The browser path (BrowserNotifier) is untouched. Updated notifier.test.ts to mock the module import (`vi.mock("@tauri-apps/plugin-notification", ...)`, the same pattern used elsewhere in this codebase for singleton-module mocking) instead of a shell global, and updated the two other test files that separately drove the desktop notifier path through the same global (rateLimitAlertRunner.test.ts, rateLimitAlertSettings.test.ts) to do the same. `npm install` under the environment's actual npm (10.9.8) produced lockfile noise unrelated to this change (a `git+ssh` vs `git+https` resolved URL for an unrelated git dependency, and dropped `libc` fields on several optional-platform packages) from a version mismatch with the project's pinned `packageManager` (npm 12.0.2); re-ran the install via `corepack npm@12.0.2` instead, which produced a clean, minimal lockfile diff adding only the new dependency and its own transitive `@tauri-apps/api`. A follow-up roborev-ci pass on this branch's own head flagged that enable() awaited notifier.requestPermission() with no rejection handling: unlike readPermission() (documented never to reject), requestPermission() has no such contract, and the desktop path's dynamic plugin import or its IPC call can genuinely fail -- an unhandled rejection out of a toggle's onchange handler with no state change or user feedback. Wrapped the request in try/catch: on failure, alerts stay off, `permission` resets to "default", and a new `requestFailed` field distinguishes this from the ordinary not-yet-decided "default" state so the settings panel can show a distinct message (`settings_rate_limit_alerts_permission_request_failed`, a new Paraglide key added across all six locales, platform-neutral since the failure can in principle happen on either notifier) instead of the neutral "you'll be asked" copy. Logged once per failure via console.error, matching this file's and notifier.ts's existing error-logging convention. One test covers the rejection path end to end (alerts stay off, permission resets, requestFailed is set, the error is logged). A further roborev-ci pass caught that loadNotificationPlugin() cached a *rejected* import() promise the same way it cached a successful one: `notificationPluginPromise ??= import(...)` only skips a reassignment, it doesn't un-remember an already-settled rejected promise, so one transient load failure (a cold module cache, a momentary asset-server hiccup) permanently wedged every later permission check and notify() call into replaying that same rejection for the rest of the session. Fixed by clearing the cached promise in a `.catch()` handler before re-throwing, so the next call starts a fresh import attempt instead of reusing the poisoned one. Tested with a dedicated fresh module instance (vi.resetModules() + a dynamic re-import, since this module-scoped cache is never reset between tests in the existing file once a load succeeds elsewhere): a first import that rejects resolves "default"; a second call, after the mock is switched to succeed, resolves "granted". usage-threshold-notifications' latest revision also dropped planType from the window key (see its own commit body). This branch's rateLimitAlertRunner.test.ts had one test hardcoding the pre-fix, plan-bearing 5-segment key as a stand-in for another tab's notified- state write; updated it to the current 4-segment shape so the mocked entry still matches the window the runner actually evaluates. A judge review (kata kemc, against the published PR) found the permission-state consolidation incomplete despite requestFailed: the store exposed a synchronous `getPermission()` placeholder write (set, then immediately overwritten once the async `readPermission()` landed) alongside the async path, and, concretely, roborev-ci reproduced a race on top of that split ownership -- the constructor's own refreshPermission() can still be in flight when enable() runs and grants; if that older, slower read is left free to write `permission` once it finally resolves, it clobbers the fresh "granted" with the stale value it observed before the grant, and the runner goes back to skipping check() until a later poll's own refreshPermission() happens to catch up. Consolidated `permission` to one write path: `refreshPermission()` no longer writes the synchronous placeholder at all (readPermission() is the only real source of truth), and every write -- from refreshPermission() or enable() -- now goes through a new `applyPermissionRead()` gated by a generation counter, so only the read that's still the latest when it resolves gets to write; an older, superseded read is discarded. The Windows denied-before-first-request handling and the rule that only enable() ever prompts (refreshPermission() only ever reads, in the background or otherwise) are unchanged. The same review asked that a permission grant actually trigger a check, not just update the field: `onPermissionGranted`, a callback the poll runner (rateLimitAlertRunner.start()) wires to its own check(), now fires whenever a permission read lands on "granted" from something else -- covering a grant that happens outside the app (OS settings) between polls, which only a background refreshPermission() would otherwise observe, not just the enable()-toggle path. enable() fires it itself, after `setEnabled(true)`, rather than firing synchronously from inside applyPermissionRead()'s own write: at the point that write happens, enable() hasn't reached setEnabled(true) yet, so an immediate callback would see the master toggle still off and check() would no-op. applyPermissionRead() instead returns whether its read became the transition into "granted", and each caller decides when it's safe to act on that. The review also asked to cut notifier.ts's comment bloat (239 lines, 108 comment-only) down to the actual platform constraints rather than the history of what was tried before landing on `window.__TAURI_INTERNALS__` detection and a dynamic plugin import (both already covered above); and to consolidate this branch's tests, which had grown to 731 added lines against roughly 374 lines of production code across notifier.ts, rateLimitAlertRunner.svelte.ts, and rateLimitAlertSettings.svelte.ts. Table-driven platform cases replaced several near-duplicate tests that separately asserted detection, selection, and the synchronous vs. async permission getters; the stale `reconcileLegacyKeys` mock and assertion in rateLimitAlertRunner.test.ts (a leftover from before usage-threshold-notifications dropped that migration -- see its own commit body -- that would have silently asserted on a call the runner no longer makes) were removed along with it. New coverage was added for the generation-gated ownership fix above (a stale, slower read must not clobber a fresher "granted") and for onPermissionGranted firing once, after `enabled` is already true, and clearing on stop(). roborev-ci on this branch's pushed PR found one more gap in the permission-consolidation work above: `enable()` only fired `onPermissionGranted` when `applyPermissionRead` reported the transition into "granted," so re-enabling the master toggle while permission was already granted (the user disabled and re-enabled, with no permission change at all) never triggered an immediate check -- an existing threshold crossing sat waiting for the next poll tick or SSE event instead of notifying right away, identically to the transition case this callback exists for. `enable()` now fires `onPermissionGranted` unconditionally on every successful return (after `setEnabled(true)`, same ordering as before), not only when `becameGranted`; the now-unused per-call `becameGranted` tracking in `enable()` was removed (the generation-gated single-owner design and `refreshPermission()`'s own transition-gated firing, for the background/OS-external-grant case, are unchanged). One test covers enabling with permission already granted invoking the callback once. Rebased onto usage-threshold-notifications' latest revision, which (among other fixes -- see its own commit body) made fireNotification() report whether delivery is known to have failed, so the runner can leave a stage eligible to retry on the next poll instead of the evaluator's "already notified" mark permanently consuming it. Extended that into this branch's `Notifier` interface: `notify()` now returns a boolean instead of `void` for both implementations -- BrowserNotifier reports its synchronous `new Notification(...)` try/catch outcome exactly as fireNotification() itself used to, and DesktopNotifier reports `isSupported()`, since its actual send stays fire-and-forget past that point (the plugin's dynamic import and IPC call resolve asynchronously, with no caller in a position to await them -- see the interface method's own doc comment for why a true async delivery outcome isn't available here). The desktop-specific tests (rateLimitAlertRunner.test.ts, notifier.test.ts) still pass unchanged, since neither exercised a failure path that distinguishes the old `void` signature from the new boolean one. A further local roborev pass found that the boolean-return fix above was itself incomplete on the desktop path: `DesktopNotifier.notify()` reported success as soon as the send was *underway* (fire-and-forget, matching its pre-existing behavior), not once it had actually resolved, so a plugin import or IPC failure surfaced only via a console.error logged after `check()` had already persisted the stage as notified -- the exact bug the boolean return was meant to close, just moved one layer down. `notify()` on both `Notifier` implementations is now `Promise<boolean>`: `BrowserNotifier`'s still resolves from its synchronous try/catch outcome (no real async work needed, just matching the interface), and `DesktopNotifier`'s awaits the plugin load and the send itself inside a try/catch, resolving to whether it actually succeeded. `fireNotification()` and `check()`'s notify loop (already inside `withCrossTabLock`) now await each delivery before deciding whether to persist a stage as notified or roll it back, so an async desktop failure is caught at the same point a synchronous browser one already was. Covered by a runner test (send rejects on the first check(), succeeds on the second) and an updated notifier.test.ts assertion on the resolved value rather than just "does not throw." A final local roborev pass found that the awaitable-notify fix above was still incomplete: tauri-plugin-notification 2.4.0's public JS API has no awaitable send at all. Its `sendNotification()` export just constructs `new window.Notification(...)`, and the plugin's own init script (which monkey-patches `window.Notification` inside the webview) fires its `invoke("plugin:notification|notify", ...)` call from inside an un-awaited async IIFE -- the constructor itself is not `async` and cannot return a promise, so nothing in the public API surface ever exposes the underlying command's outcome. `await`ing `sendNotification()` therefore only ever awaited its own synchronous return; a real IPC failure happened entirely out of band. `notify()` now bypasses the plugin's JS layer and calls the underlying `plugin:notification|notify` Tauri command directly via `@tauri-apps/api/core`'s `invoke()` (added as a direct dependency at the version already resolved transitively through the plugin, `2.11.1`; `corepack npm@12.0.2 install`, per this branch's established convention, produced a one-line lockfile diff), with the same `NotificationData`-shaped payload (`id`/`title`/`body`) the plugin's own init script sends -- confirmed by reading both the plugin's dist-js/index.js (`sendNotification`'s actual body) and its Rust `commands.rs`/`init-iife.js` sources. This is now a genuinely awaitable command whose rejection reaches `notify()`'s existing try/catch. Updated notifier.test.ts and rateLimitAlertRunner.test.ts to mock `@tauri-apps/api/core`'s `invoke` instead of the plugin's own (now-unused) `sendNotification` export. The same pass also caught that `readPermission()` still couldn't reliably observe an externally-granted permission, for a subtler reason than notify()'s: the plugin's `isPermissionGranted()` JS wrapper caches `window.Notification.permission` and only re-queries the OS while that cache still reads "default" (`if (window.Notification.permission !== 'default') return Promise.resolve(...)`, in the plugin's own init script) -- once the cache has been set once, at init or after a request, every later call returns that SAME stale value, no matter how many times `refreshPermission()` polls it. A permission later granted through the OS outside the app would therefore never be observed, silently undermining the whole point of `onPermissionGranted`/background polling this branch just built. `readPermission()` now invokes `plugin:notification|is_permission_granted` directly for the same reason `notify()` bypasses the plugin's JS layer (see DesktopNotifier's doc comment, and its Rust command in `commands.rs`, confirming the return type is `Option<bool>` -- prompt/granted/denied). That also fixed a latent, independent bug in the old mapping: `(await isPermissionGranted()) ? "granted" : "denied"` read a `null` ("prompt", i.e. undecided) result as "denied" rather than "default", since `null` is falsy in JS. Rewired notifier.test.ts and rateLimitAlertSettings.test.ts's desktop permission tests to mock `@tauri-apps/api/core`'s `invoke` in place of the plugin's own `isPermissionGranted` export, and added a regression test asserting two different resolved values across two calls (standing in for a permission that changed between them, which the old cached-wrapper approach could never surface). Roborev-ci finding on the delivery loop in rateLimitAlertRunner.svelte.ts's check(): each notification in result.toNotify is awaited one at a time, inside the cross-tab lock, so a settings change landing between two of those awaits (the user disabling alerts mid-batch) was never rechecked -- every later send in the same batch still fired and got persisted as delivered, regardless. The loop now rechecks the live `enabled` flag before each send; once it flips, the remaining sends are skipped (not attempted) and roll back the same way a failed delivery already did, leaving them eligible for a later cycle instead of firing on skipped-past alerts. Added a regression test that flips `enabled` off from inside the first notification's own mocked `invoke` call and asserts the second, still-queued notification is never attempted and stays unmarked.
jzila
force-pushed
the
desktop-notifications
branch
from
September 11, 2026 07:26
61041d1 to
17e8b9d
Compare
roborev: Combined Review (
|
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
1 participant
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.
Stacked on #1705, which is stacked on #1704. The desktop-specific work is the commits from
feat(desktop): deliver rate-limit notifications via tauri-plugin-notificationonward.The Tauri desktop app loads the same frontend inside a webview, where the browser Notification API is not what a user sees as a system notification. This adds
tauri-plugin-notificationto the shell and aDesktopNotifieron the frontend that the notifications runner selects whenwindow.__TAURI__is present, so a rate-limit alert arrives as a native macOS, Windows or Linux notification instead of a webview one.Permission handling differs by platform in ways worth knowing. The plugin's Windows shim reports permission as denied before anything has been asked, so the desktop path requests permission whenever it is not already granted rather than only when the state is undecided. Reading permission can also reject on some platforms; the notifier catches that, logs it and reports undecided so the enable flow does not get stuck, and the runner retries the read without prompting on each poll while the user has notifications enabled but not yet granted. The browser path keeps its narrower behavior of asking only when the state is undecided.
The capability file grants the notification permission to the main window only. Locale strings for the permission states were made platform-neutral, since the same copy is shown in both the browser and the desktop shell.
Reviewers should look at
frontend/src/lib/utils/notifier.tsfor the two notifier implementations and the platform gate,desktop/src-tauri/src/lib.rsandcapabilities/default.jsonfor the shell wiring, and the runner change that retries the permission read.