fix(desktop): allow hiding the status bar - #63049
Conversation
6da159a to
bfeddf6
Compare
|
Hi @CrucisLee, @jamesxia1988, and @marsmensch — thank you for the thoughtful issues and prior work around the Desktop status bar. This is my first contribution to Hermes Agent, and I’m really happy to contribute to a project I’ve enjoyed exploring. I learned a lot while working through the Desktop configuration, UI, accessibility, and localization paths. This PR addresses #62981 by adding three status bar modes:
I also reviewed the related readability work in #61535/#63028 and the tooltip work in #41967. I’ve kept this change focused on visibility controls so those visual and tooltip improvements remain compatible and can evolve independently. Since this is my first contribution here, I’d especially appreciate any feedback on the implementation, accessibility behavior, naming, or project conventions. Thanks for taking the time to review it! |
CrucisLee
left a comment
There was a problem hiding this comment.
Review as the original issue author (#62981)
Thanks for picking this up — happy to see it land. Walked through the full diff and a few notes from the issue author's perspective:
What works well
offmode returningnullrather thandisplay: none-ing an existing element is the right call — the timer really isn't there, not "hidden but still in the layout." This is exactly what the issue asked for in the "持续视觉压力" framing.auto-hideuses CSStranslate-y-full+group-hover/statusbar+group-focus-within/statusbar, no JS timer. Clean, GPU-friendly, no setInterval leak risk.- Optimistic update with rollback in
persistDesktopStatusbarModeis the right pattern for "settings that should feel instant." Good test coverage for the rollback path too. - Default
onpreserves upgrade compatibility — sensible. - Translation coverage across en / zh / zh-Hant / ja is appreciated.
- The
comp/desktoplabel now exists in the repo (didn't when the issue was filed), and you used it correctly.
Two things to consider (non-blocking)
-
Merge conflict. The branch shows
mergeable: CONFLICTING— likely just drift from main. A rebase should clear it. -
Keyboard reachability of the reveal zone. The 2px-tall
<div data-slot="statusbar-reveal-zone" tabIndex={0}>is keyboard-focusable, and the focus-within transition should fire on Tab — but I haven't run it locally to confirm the focus chain actually works in the real Electron shell. If it doesn't, the screen-reader / keyboard-only user experience would silently regress compared to the previous "always visible" behavior. Worth a manual pass:- Tab through the chat surface — does the reveal zone receive focus visibly?
- Once focused, does the status bar slide up? (CSS
group-focus-within/statusbarshould handle this, but a smoke check is cheap.)
If you'd rather not block on this, the fallback is a dedicated
Cmd+.shortcut that togglesauto-hide → on → auto-hide, but I don't think it's required for this PR.
Not a concern
- No new dependencies
- TUI
/statusbarindependence preserved isSecondaryWindow()still skips the bar for pop-out windows- No prompt-cache implications (display config, not conversation state)
Happy to approve once the merge conflict is resolved. Thanks again for the thorough fix.
bfeddf6 to
9b5a0ca
Compare
|
Thanks so much for the thoughtful review, @CrucisLee ! |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused accessibility improvement. Current main still always mounts the Desktop status bar at apps/desktop/src/app/shell/app-shell.tsx:220, so the feature remains needed.
Problems
- In
auto-hide, the footer is revealed only bygroup-hover/statusbar/group-focus-within/statusbar(apps/desktop/src/app/shell/statusbar-controls.tsx:79-80). Status-bar controls include dropdown menus (apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx:248-304,:360-395), butDropdownMenuContentis rendered through a Radix portal outside that group (apps/desktop/src/components/ui/dropdown-menu.tsx:77-94). Moving focus or the pointer into an open menu retracts the footer while the menu is active.
Suggested changes
- Retain the auto-hidden footer while a status-bar dropdown is open, and add a regression test covering focus inside a portaled status-bar menu.
Automated hermes-sweeper review.
| return ( | ||
| <div | ||
| aria-label={t.settings.appearance.statusbarReveal} | ||
| className="group/statusbar absolute inset-x-0 bottom-0 z-30 h-2 outline-none [-webkit-app-region:no-drag]" |
There was a problem hiding this comment.
The reveal group does not contain Radix menu content: DropdownMenuContent portals under document.body. Once a status-bar menu opens and focus/pointer moves into it, these group selectors stop matching and retract the footer. Please retain visibility while any status-bar dropdown is open, with a regression test for that portal-focus path.
9b5a0ca to
0d0ce4d
Compare
|
Thanks for catching this, @teknium1 — I’ve fixed it in the latest update. |
74ba906 to
7256ca2
Compare
|
Thanks for the thorough follow-up, @Coooder-Crypto — both the merge conflict and the keyboard-focus path I flagged in my earlier review are addressed here, and Teknium’s portal concern (Radix menu content rendering under LGTM from my side — happy to see this land. 🎉 |
|
@teknium1, looks good from my side. It appears there aren’t any check runs on the branch yet—could you trigger the usual CI before merge when convenient? |
9820377 to
ca518bf
Compare
ca518bf to
cdbdc7d
Compare
|
@Coooder-Crypto — heads up on a development since my last review: Hermes v0.20.0 (shipped Aug 3) landed "status bar hideable" on main — a per-item right-click toggle in the status bar that lets users hide the session timer (and other items) individually (#68140/#72960). That covers the core accessibility ask from #62981, which is why I was tracking this PR. Your whole-bar approach is still the more complete UX — Two paths, your call:
Either way, thanks again for the careful work — the portaled-menu fix and its regression test were excellent. |
Summary
display.desktop_statusbarwithon,off, andauto-hidemodes/statusbarsettingWhy
The Desktop status bar includes a continuously updating session timer. This can be distracting or stressful for users who prefer a quieter workspace, while the TUI already provides control over its own status bar.
The setting defaults to
off, matching the current opt-in status-bar behavior onmain. Existing users who already have the released local visibility preference keep their choice through a one-time profile-config migration. Hiding the status bar does not hide the composer, send button, model picker, or other input controls.Validation
Fixes #62981