Surface the taskbar widget's hidden state in Settings - #264
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe taskbar widget now exposes typed availability and activity states through Tauri. Native status changes emit events, transient landmark failures use six-cycle debouncing, and the settings UI displays status-specific guidance with native and frontend test coverage. ChangesTaskbar widget status
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskbarWidget
participant Tauri
participant SettingsSection
TaskbarWidget->>Tauri: emit TASKBAR_WIDGET_STATUS_CHANGED
SettingsSection->>Tauri: invoke get_taskbar_widget_status
Tauri->>TaskbarWidget: get_taskbar_widget_status
TaskbarWidget-->>Tauri: TaskbarWidgetStatus
Tauri-->>SettingsSection: TaskbarWidgetStatus
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop-tauri/src-tauri/src/taskbar_widget.rs (1)
690-696: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport
NoProviderswhen native mode is enabled without a configured provider.When
native_mode_enabled(settings)is true andnative_mode_has_configured_provider(settings)is false, this branch setsTaskbarWidgetStatus::Disabled. The Settings UI hidesDisabled, so users do not receive the required no-provider guidance. SetTaskbarWidgetStatus::NoProvidersfor this case.Proposed fix
- if native_mode_enabled(settings) && native_mode_has_configured_provider(settings) { + if native_mode_enabled(settings) && native_mode_has_configured_provider(settings) { schedule_recovery(app); } else { hide_existing(); TRANSIENT_STREAK.store(0, Ordering::Release); - set_status(app, TaskbarWidgetStatus::Disabled); + let status = if native_mode_enabled(settings) { + TaskbarWidgetStatus::NoProviders + } else { + TaskbarWidgetStatus::Disabled + }; + set_status(app, status); }🤖 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 `@apps/desktop-tauri/src-tauri/src/taskbar_widget.rs` around lines 690 - 696, Update the native-mode status branching around native_mode_enabled and native_mode_has_configured_provider so enabled mode without a configured provider calls set_status with TaskbarWidgetStatus::NoProviders instead of Disabled. Preserve the existing recovery behavior for configured providers and cleanup/reset behavior in the fallback path.
🤖 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 `@apps/desktop-tauri/src/floatbar/SettingsSection.tsx`:
- Around line 125-135: Update refreshTaskbarStatus in the useEffect to track an
incrementing request sequence and apply a resolved status only when it is from
the latest request and the effect is not cancelled. Preserve the existing
last-known status behavior on failures, and add a deferred-promise test that
resolves two status reads in reverse order to verify the older result is
ignored.
---
Outside diff comments:
In `@apps/desktop-tauri/src-tauri/src/taskbar_widget.rs`:
- Around line 690-696: Update the native-mode status branching around
native_mode_enabled and native_mode_has_configured_provider so enabled mode
without a configured provider calls set_status with
TaskbarWidgetStatus::NoProviders instead of Disabled. Preserve the existing
recovery behavior for configured providers and cleanup/reset behavior in the
fallback path.
🪄 Autofix
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: ca5acdc8-ab56-4cae-8806-081b263111cb
📒 Files selected for processing (8)
apps/desktop-tauri/src-tauri/permissions/commands.tomlapps/desktop-tauri/src-tauri/src/events.rsapps/desktop-tauri/src-tauri/src/main.rsapps/desktop-tauri/src-tauri/src/taskbar_widget.rsapps/desktop-tauri/src/floatbar/SettingsSection.test.tsxapps/desktop-tauri/src/floatbar/SettingsSection.tsxapps/desktop-tauri/src/lib/tauri.tsapps/desktop-tauri/src/types/bridge.ts
ae675f1 to
e258d37
Compare
|
Both review findings are addressed in e258d37:
The branch is also rebased onto current |
|
This is really nicely done — thank you. Pulling the status mapping and debounce out into pure functions so a Win32-bound module can actually be unit-tested is the part I appreciated most, and the Heads up that 1.5.29 landed changes in Two small things, neither blocking:
Happy to push (1) myself if you'd rather not bother, but it's your PR and it's a small change, so I'd rather leave it with you. |
|
Thanks for merging main in and re-running everything, appreciated. On (1): agreed, and it's a good catch, a permanently silent status row is the one failure mode this feature exists to remove, so it shouldn't be able to inflict it on itself. Pushed 6eda2a1: On (2): you're right, the sentence overclaimed. An existing |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Report NoProviders instead of Disabled when native mode is enabled without a configured provider, and guard the Settings status row against out-of-order reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A poisoned STATUS lock made set_status report "no change" and current_status fall back to Disabled, hiding the status row permanently — the silent failure the row exists to prevent. Recover the guard with unwrap_or_else(PoisonError::into_inner) in both spots so a panic elsewhere degrades to a possibly-stale but visible status, with a test poisoning the lock from another thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6eda2a1 to
f64771c
Compare
…fit the widget (#281) Fixes the silent-hide from #261, per the plan in that thread: fix `auto` first, its own PR, no new setting. Opening this a bit ahead of #264 settling: manual verification turned up a worse shipping manifestation of the same bug (the Widgets freeze below) that seemed worth showing sooner. Once #264 lands I'll rebase and generalize its status-row string ("between Widgets and Start" → wording that covers both lanes) — one small hunk; there's no other coupling between the two PRs. On Windows 11 with **Taskbar alignment = Left** (a stock setting), Start sits at x≈0 and the Widgets→Start lane's right edge goes negative, so the native widget hides silently. Windhawk's "Start button always on left" triggers the same thing. This PR keeps that lane as the always-preferred first choice and adds a fallback: when it has no verified gap that clears the minimum width, try a second lane between Start and the tray, with exactly the same obstacle verification — pinned/running icons are obstacles, so the widget lands centered in the empty stretch after the last icon, never covering a button. ## What changed - `floatbar/taskbar.rs`: the `TrayNotifyWnd` rect (already discovered via `FindWindowExW` and used as an obstacle) is now also exposed as `landmarks.tray`. It bounds the fallback lane's right edge; when a taskbar doesn't expose it (secondary taskbars can omit it) or its rect fails the taskbar-band sanity check, the lane falls back to the taskbar's own right edge. - `taskbar_widget.rs`: the inline gap-scan in `placement_outcome` is factored into a `best_gap` helper (same obstacle filter, same scan, same largest-gap tie-break — a pure refactor), and lane 2 is tried only when lane 1 returns no qualifying gap. Placement in either lane is the existing policy: centered in the largest fully-empty gap, `desired.min(available)` width. ## What deliberately didn't change - Lane 1 wins whenever it fits at all, so behavior on centered-alignment taskbars is byte-identical — all 37 pre-existing placement policy tests pass untouched, and a new test pins lane 1 winning even when lane 2 also has room. - No settings, no UI. (The #264 status row's "between Widgets and Start" wording will be generalized in a follow-up hunk once that PR is in main.) - One guard that predates this PR did have to change, because manual verification proved it breaks the headline scenario: with alignment = Left **and Windows Widgets enabled (the stock default)**, Windows renders the Widgets entry by the tray, so UIA reports it right of Start permanently. The existing `widgets.right >= start.left` guard reads that as mid-animation and returns `TransientLandmarks` on every pass — and since transient misses deliberately preserve the existing window, a widget placed before flipping alignment stays frozen on top of the app icons indefinitely. This isn't introduced by the branch: the guard has shipped since v0.43 (a01eaab) and the preservation policy is similarly long-standing. Reproduced on the installed 1.5.27 release: with Widgets off, flipping to left alignment makes the strip vanish (the known #261 silent hide); with Widgets on, it instead stays frozen on top of the app icons (screenshot below). Now that geometry means "lane 1 doesn't exist": lane 2 places the widget and the Widgets rect joins the obstacle set, so the verified gap can never cover it. Genuinely stale (off-band) Widgets rects keep the transient policy, pinned by its own test. - The obstacle model is unchanged: XAML controls are harvested via UIA with the existing Button-type filter, same as lane 1 has always used. Lane 2 traverses new territory (right of Start), so anything there that UIA doesn't expose as a button — worth watching: the wide search box, and the clock on secondary taskbars without `TrayNotifyWnd` — wouldn't be avoided. Verified on real hardware (Windows 11 Pro 25H2, build 26200.9168): with the search box enabled under left alignment, UIA exposes the pill as a button and the widget avoids it like any other control. If a configuration surfaces where UIA under-reports, extending the harvest filter is a contained follow-up in `uia_buttons`. One behavior in lane 2 is deliberately stricter than "missing tray": a tray rect that fails the taskbar-band sanity check is treated as *transient* (stale rect mid layout/DPI change — retry next watchdog pass) rather than widening the lane to the taskbar's right edge over the tray's true position. Same staleness policy as an off-band Start or Widgets rect. ## New tests Ten: the left-aligned-Start fallback itself (the test that closes #261), missing-tray right-edge fallback, off-band tray reporting transient, the tray-side Widgets entry becoming a lane-2 obstacle, off-band Widgets staying transient, lane 1 preferred when both lanes fit, crowded-but-non-negative lane 1 falling through, fully-obstructed lane 2 still reporting `VerifiedNoFit`, the vertical-taskbar early return, and multi-monitor with per-taskbar tray rects. Placement tests assert hand-computed pixel positions, not just presence. ## Commands run ``` cargo fmt --all cargo test --manifest-path rust\Cargo.toml cargo test --manifest-path apps\desktop-tauri\src-tauri\Cargo.toml cargo clippy --manifest-path rust\Cargo.toml --all-targets -- -D warnings cargo clippy --manifest-path apps\desktop-tauri\src-tauri\Cargo.toml --all-targets -- -D warnings pnpm --dir apps/desktop-tauri test ``` All green: 872 + 504 Rust tests, 500 frontend tests, clippy clean with `-D warnings`. Manual verification on Windows 11 Pro 25H2 (build 26200.9168), single monitor, with and without Windhawk mods. ## Screenshots **Alignment = Center (default) — placement unchanged:** <img width="3439" height="40" alt="normal" src="https://github.com/user-attachments/assets/7dad8966-c9d4-4781-9fe7-670ea8e5a445" /> **Taskbar alignment = Left (stock Windows 11, no mods) — previously hidden silently, now centered in the free stretch between the icons and the tray:** <img width="3439" height="41" alt="taskbar-aligment-left" src="https://github.com/user-attachments/assets/1fb228c0-e36e-41d7-8b26-5de0a8c77855" /> **Windhawk "Start button always on left" (icons stay centered) — same fallback lane, but here the largest verified gap is between Start and the centered icons, so the widget lands there and nearly recreates the default look. The fallback isn't "move right", it's "find the room":** <img width="3439" height="41" alt="windhawk-start-button-always-on-the-left" src="https://github.com/user-attachments/assets/e78d548f-c8a9-49e3-9d74-1674c6bf99ad" /> **Transition honesty: re-placement is watchdog-driven (5s), so a violent relayout like flipping alignment can overlap icons until the next pass. Pre-existing cadence, unchanged by this PR — the same staleness exists on main whenever the icon row grows:** <img width="3439" height="40" alt="5-seconds-window" src="https://github.com/user-attachments/assets/a4a0c647-d8d8-4a39-9e24-2f72535b1a8f" /> **The pre-existing Widgets-guard freeze (before): alignment = Left with Windows Widgets on — the guard reports transient forever and the preserved widget stays frozen over the icons. Ships in every release since v0.43; fixed by this PR. (The weather entry itself is also misplaced in this shot — that's an Explorer relayout quirk when flipping alignment center→left with Widgets on; it reproduces with Ceiling closed and is unrelated to this change.):** <img width="3439" height="41" alt="windows-widget-bug" src="https://github.com/user-attachments/assets/2abc63df-5973-4450-926d-bd20ed1a52af" /> **After the fix: same stock-default combo, widget placed in the fallback lane with the Widgets entry treated as an obstacle:** <img width="3439" height="40" alt="windows-widget-reactivated" src="https://github.com/user-attachments/assets/a0e42d4a-89b6-4c2e-b851-837e05abd8da" /> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved widget placement around the Windows taskbar and notification area. * Prevented widgets from overlapping taskbar controls, tray icons, or other obstructions. * Improved placement on left-aligned and vertical taskbars. * Added more reliable fallback positioning when taskbar information is unavailable or outdated. * Improved placement across multiple monitors, including monitor-specific notification areas. * Simplified the taskbar status message when no suitable space is available. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: tsouth89 <tsouth89@users.noreply.github.com> Co-authored-by: Tyler <tsouth2@gmail.com>
Cuts 1.5.30. Version bumped in all five locations from `VERSIONING.md`, `BUILD_NUMBER=132`, `Cargo.lock` regenerated, and the `Unreleased` entries moved into a dated section. ## What's in it **A second click on the tray icon hides the dashboard** (#283, closes #280). Left-click always reopened the stats window. First click shows it, second click hides it. A Windows double-click is ignored so the window does not flash. **Grok banked resets show up** (#282). Codex already had the chip. Grok's redeemable reset tokens now appear on Overview, provider detail, and the taskbar flyout. Redeem stays on grok.com. **Settings says why the taskbar widget is hidden** (#264). The strip could vanish with no explanation. The Taskbar Usage group now reports shown, no free space, waiting for landmarks, or no enabled providers. **The taskbar widget takes a second lane when the usual one does not fit** (#281, closes #261). Left-aligned taskbars (and Windhawk "Start always on left") made the Widgets-to-Start gap go negative, so the strip hid. If that lane has no room, it now uses the stretch between Start and the tray. **Charts no longer mix one account's usage into another** (#277). A removed account could inherit the machine-wide cache. Each account now has its own tab and cache identity. **Provider detail no longer applies a stale refresh** (#278). A slower request from the previous seat could paint over the one you just switched to. **Credential status is scoped to the selected provider** (#279). Opening provider B could show a protected store and revoke action that belonged to provider A. ## Validation `scripts\local-check.ps1 -All -Version 1.5.30` passed except the known local PTY failure: - `cli::tty_runner::tests::test_run_sends_script_through_pty` fails locally because `%APPDATA%\npm` has a `cmd` shim that is not a valid Win32 application. Same environment issue noted on #263. 877 other shared Rust tests passed. Tauri Rust tests (517), frontend tests (525), frontend build, clippy, fmt, store-submission prep, and release-doctor all passed. Three release-doctor warnings are expected pre-tag: no local `v1.5.30` tag, no GitHub release yet, no local assets directory. ## Affected areas - [x] Tray panel - [x] Settings UI - [ ] Config file / settings persistence - [ ] CLI - [x] Provider-specific behavior - [x] Installer / release packaging - [ ] Startup / background behavior - [x] Documentation - [ ] Other: ## UI / tray proof - [x] Visual proof was not practical; manual validation and explanation attached The tray toggle was click-tested on a local debug build of `main` before this version bump. Release notes are the changelog section in this PR. ## Notes for reviewers This PR is the version bump and changelog only. The signed installer is built by tagging `v1.5.30` after merge. Co-authored-by: tsouth89 <tsouth89@users.noreply.github.com>
First step toward #261: the native taskbar strip can be hidden by placement outcomes (
VerifiedNoFit, persistentTransientLandmarks) with no feedback anywhere, which makes it look broken — especially under taskbar mods. This PR makes those states visible in Settings without changing any placement behavior or defaults.What changed
prepare_widgets' stringly errors became a typedPrepareFailureenum; the watchdog's completion path maps each pass to aTaskbarWidgetStatusstored next to the host state. Transient landmark misses are debounced (6 consecutive watchdog ticks, ~30s) before reporting "waiting", mirroring the existing keep-the-widget-visible policy, so a Start/Search flyout opening never flashes the row.taskbar-widget-status-changedevent (same idiom assettings-changed) plus aget_taskbar_widget_statuscommand; the row re-fetches on event. Emitted only on actual status transitions.apply_preparedsucceeds, so the row can never newly claim "shown" when the Win32 side failed; an existing "Shown" status persists through a transient apply failure until the next conclusive outcome (the watchdog retries every 5s).No placement logic changed; all pre-existing tests pass unmodified. New unit tests cover the status mapping and debounce (pure, no Win32) and the Settings row rendering per status.
Interesting data point while validating: with Windhawk's "Start button always on left" active, the reported state is the no-fit one, not waiting — Start stays discoverable when pinned left; it's the collapsed lane that hides the strip. That matches the analysis in #261.
Commands run
All green: 872 + 491 Rust tests, 488 frontend tests, clippy clean with
-D warnings,tsc+ vite build OK.Screenshots
Widget visible (normal taskbar):
Widget hidden under Windhawk "Start button always on left" — previously silent, now explained:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes