feat(browser): redesign to the design bar with a collapsible sidebar (#66)#1227
Conversation
…(#66) Brings the Browser app up to Images Studio / design-bar quality: - Collapsible left sidebar (BrowserSidebar.tsx) -- expanded 220px shows tab titles + favicons with active-indicator rail; collapsed 44px is icon-only. Width animates via CSS transition (respects prefers-reduced-motion and data-perf=reduced). State lives in a lightweight zustand store (browser-ui-store.ts) so it persists within a session without touching localStorage or the session-persistence snapshot. - Polished empty/connecting states (BrowserEmptyState.tsx) replacing the plain white void: new-tab shows a Globe + "New tab" prompt; the Neko connecting state shows an animated MonitorPlay icon with loading dots. DiscardedPlaceholder also refined to match the design bar's type hierarchy. - TabStrip tightened: reduced gap, smaller close button, tab border now uses a surface-inset shadow so active tabs merge cleanly into the content area. New-tab button gets a focus-visible ring. - Chrome toolbar gap tightened to 1.5 (from 2) for a denser, more intentional feel consistent with the images/store toolbars. - Sidebar CSS keyframe animations added to tokens.css alongside the existing taos-shimmer / taos-card-enter animations. All 10 BrowserApp tests pass. LiveBrowserView iframe/neko_url logic is untouched. Mobile layout is untouched.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds a collapsible ChangesBrowserApp Collapsible Sidebar and Empty States
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| export function BrowserEmptyState({ variant }: BrowserEmptyStateProps) { | ||
| if (variant === "connecting") { | ||
| return ( | ||
| <div className="absolute inset-0 flex flex-col items-center justify-center gap-4 bg-shell-bg-deep select-none"> | ||
| <span | ||
| className="browser-stream-pulse flex h-[52px] w-[52px] items-center justify-center rounded-2xl bg-shell-surface border border-shell-border text-shell-text-tertiary" | ||
| aria-hidden="true" | ||
| > | ||
| <MonitorPlay size={24} /> | ||
| </span> | ||
| <div className="flex flex-col items-center gap-1 text-center"> | ||
| <p className="text-[13px] font-semibold text-shell-text"> | ||
| Starting full browser session | ||
| </p> | ||
| <p className="text-[11.5px] text-shell-text-tertiary"> |
There was a problem hiding this comment.
💡 Quality: "connecting" empty-state variant is never rendered (dead code)
BrowserEmptyState supports a "connecting" variant (animated MonitorPlay + loading dots), and the PR description states "the Neko connecting state shows an animated MonitorPlay icon with loading dots." However, the only call site renders variant="new-tab" (TabRenderer.tsx:508). When tab.liveSession is set, TabRenderer renders <LiveBrowserView> directly (lines 294-307) without ever showing the connecting placeholder. As a result the connecting branch is dead code and the advertised connecting UX is not actually wired up.
Either wire the connecting state in (e.g. render <BrowserEmptyState variant="connecting" /> while the Neko stream is still establishing, before/over LiveBrowserView), or drop the unused variant to avoid misleading dead code.
Render the connecting placeholder until the stream is connected (adjust the readiness flag to match LiveBrowserView's actual API).:
// In TabRenderer, while a live session is starting, overlay the connecting state.
if (isActive && tab.liveSession) {
return (
<div key={tab.id} style={{ position: "absolute", inset: 0 }} data-window-tab={tab.id}>
<LiveBrowserView
nekoUrl={tab.liveSession.nekoUrl}
streamToken={tab.liveSession.streamToken}
/>
{!tab.liveSession.connected && <BrowserEmptyState variant="connecting" />}
</div>
);
}
Was this helpful? React with 👍 / 👎
| <aside | ||
| aria-label="Tabs sidebar" | ||
| data-collapsed={collapsed ? "true" : "false"} | ||
| className="browser-sidebar flex flex-col flex-none bg-shell-bg-deep border-r border-shell-border overflow-hidden" |
There was a problem hiding this comment.
💡 Quality: Sidebar rows use role=option without listbox parent or keyboard support
SidebarTabRow renders a <div role="option" aria-selected={...}> with an onClick activation, but the containing <aside> is not a role="listbox" (it is a plain landmark). role="option" is only valid inside a listbox/combobox, so screen readers will not announce these rows correctly. Additionally the row is a non-focusable <div> with no tabIndex and no onKeyDown, so keyboard users cannot activate a tab from the sidebar (the close button is explicitly tabIndex={-1}). Consider using the existing role="tablist"/role="tab" pattern (as TabStrip does) or making rows focusable buttons with keyboard handlers, and giving the scroll container the matching container role.
Add focusability and keyboard activation; wrap rows in a role="listbox" container.:
// Make the row a real button/focusable element with keyboard activation,
// and give the list container role="listbox" (or use tablist/tab).
<div
role="option"
tabIndex={0}
aria-selected={isActive}
onClick={onActivate}
onKeyDown={(e) => {
if (e.key === "Enter" || e.key === " ") { e.preventDefault(); onActivate(); }
}}
...
>
Was this helpful? React with 👍 / 👎
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review 👍 Approved with suggestions 0 resolved / 2 findingsImplements a collapsible browser sidebar with updated empty states and refined tab strip styling. Address the unused 'connecting' empty-state variant and ensure sidebar rows are wrapped in a listbox with proper keyboard navigation support. 💡 Quality: "connecting" empty-state variant is never rendered (dead code)📄 desktop/src/apps/BrowserApp/BrowserEmptyState.tsx:17-31 📄 desktop/src/apps/BrowserApp/TabRenderer.tsx:294-307 📄 desktop/src/apps/BrowserApp/TabRenderer.tsx:506-509 BrowserEmptyState supports a Either wire the connecting state in (e.g. render Render the connecting placeholder until the stream is connected (adjust the readiness flag to match LiveBrowserView's actual API).💡 Quality: Sidebar rows use role=option without listbox parent or keyboard support📄 desktop/src/apps/BrowserApp/BrowserSidebar.tsx:36-39 📄 desktop/src/apps/BrowserApp/BrowserSidebar.tsx:156-170
Add focusability and keyboard activation; wrap rows in a role="listbox" container.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@desktop/src/apps/BrowserApp/BrowserSidebar.tsx`:
- Around line 157-163: The sidebar tab rows lack keyboard accessibility in two
ways: the clickable div at line 157 with role="option" needs keyboard event
handling to support Enter and Space key presses for activating tabs, and the
close-button element in the 205-213 range has tabIndex={-1} which removes it
from keyboard focus. To fix, add an onKeyDown handler to the div element that
checks for Enter or Space key events and calls onActivate to make tab activation
keyboard-operable, and remove the tabIndex={-1} attribute (or set it to 0) from
the close-button element to restore keyboard accessibility for closing tabs.
In `@desktop/src/apps/BrowserApp/TabStrip.tsx`:
- Around line 174-176: The className string for the close button is removing the
default focus outline with focus-visible:outline-none without providing an
alternative visible focus indicator, which breaks keyboard accessibility. Add a
replacement focus-visible class (such as focus-visible:ring-1 with an
appropriate ring color) to the className array to ensure keyboard users can
clearly see when the close button has keyboard focus. This replacement should
provide a visible ring or outline that makes the focused state obvious to
keyboard navigation users.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c177bca-59ab-4561-b0d2-d6d995f14d61
📒 Files selected for processing (8)
desktop/src/apps/BrowserApp/BrowserApp.tsxdesktop/src/apps/BrowserApp/BrowserEmptyState.tsxdesktop/src/apps/BrowserApp/BrowserSidebar.tsxdesktop/src/apps/BrowserApp/Chrome.tsxdesktop/src/apps/BrowserApp/TabRenderer.tsxdesktop/src/apps/BrowserApp/TabStrip.tsxdesktop/src/stores/browser-ui-store.tsdesktop/src/theme/tokens.css
| <div | ||
| role="option" | ||
| aria-selected={isActive} | ||
| aria-label={title} | ||
| onClick={onActivate} | ||
| title={title} | ||
| className={[ |
There was a problem hiding this comment.
Sidebar tab rows are not keyboard-operable.
Line 157 uses a clickable div without keyboard interaction, and Line 212 removes close-button tab focus (tabIndex={-1}). This makes sidebar tab activation/closing inaccessible from keyboard.
Suggested fix
- <div
- role="option"
+ <button
+ type="button"
+ role="option"
aria-selected={isActive}
aria-label={title}
- onClick={onActivate}
+ onClick={onActivate}
title={title}
className={[
"group relative flex items-center gap-2 cursor-pointer select-none",
"transition-colors focus-visible:outline-none",
collapsed ? "mx-2 my-0.5 h-[32px] w-[28px] rounded-lg justify-center" : "mx-1.5 my-0.5 h-[32px] rounded-lg px-2",
isActive
? "bg-shell-surface-active text-shell-text"
: "text-shell-text-secondary hover:bg-white/[0.04] hover:text-shell-text",
].join(" ")}
>
@@
- tabIndex={-1}
+ tabIndex={0}
@@
- </div>
+ </button>Also applies to: 205-213
🤖 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 `@desktop/src/apps/BrowserApp/BrowserSidebar.tsx` around lines 157 - 163, The
sidebar tab rows lack keyboard accessibility in two ways: the clickable div at
line 157 with role="option" needs keyboard event handling to support Enter and
Space key presses for activating tabs, and the close-button element in the
205-213 range has tabIndex={-1} which removes it from keyboard focus. To fix,
add an onKeyDown handler to the div element that checks for Enter or Space key
events and calls onActivate to make tab activation keyboard-operable, and remove
the tabIndex={-1} attribute (or set it to 0) from the close-button element to
restore keyboard accessibility for closing tabs.
| "flex h-[16px] w-[16px] items-center justify-center rounded-[4px] shrink-0 text-shell-text-tertiary transition-opacity hover:bg-white/[0.08] hover:text-shell-text focus-visible:outline-none", | ||
| isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100", | ||
| ].join(" ")} |
There was a problem hiding this comment.
Close button lost visible keyboard focus state.
Line 174 removes visible focus styling (focus-visible:outline-none without a replacement ring). Keyboard users won’t get a reliable focus indicator here.
Suggested fix
className={[
- "flex h-[16px] w-[16px] items-center justify-center rounded-[4px] shrink-0 text-shell-text-tertiary transition-opacity hover:bg-white/[0.08] hover:text-shell-text focus-visible:outline-none",
+ "flex h-[16px] w-[16px] items-center justify-center rounded-[4px] shrink-0 text-shell-text-tertiary transition-opacity hover:bg-white/[0.08] hover:text-shell-text focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent/40",
isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100",
].join(" ")}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "flex h-[16px] w-[16px] items-center justify-center rounded-[4px] shrink-0 text-shell-text-tertiary transition-opacity hover:bg-white/[0.08] hover:text-shell-text focus-visible:outline-none", | |
| isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100", | |
| ].join(" ")} | |
| "flex h-[16px] w-[16px] items-center justify-center rounded-[4px] shrink-0 text-shell-text-tertiary transition-opacity hover:bg-white/[0.08] hover:text-shell-text focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent/40", | |
| isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100", | |
| ].join(" ")} |
🤖 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 `@desktop/src/apps/BrowserApp/TabStrip.tsx` around lines 174 - 176, The
className string for the close button is removing the default focus outline with
focus-visible:outline-none without providing an alternative visible focus
indicator, which breaks keyboard accessibility. Add a replacement focus-visible
class (such as focus-visible:ring-1 with an appropriate ring color) to the
className array to ensure keyboard users can clearly see when the close button
has keyboard focus. This replacement should provide a visible ring or outline
that makes the focused state obvious to keyboard navigation users.
…idebar a11y (#124) Three deferred nits from the streamed-browser PRs (#1227/#1228). - browser_container.py: wrap build_neko_run_args() in asyncio.to_thread() at the async call site so the blocking tailscale ip subprocess (and any netifaces fallback) runs off the event loop. The sync function stays unchanged, keeping all existing tests working. - TabRenderer.tsx: introduce LiveSessionSlot — a small wrapper that renders LiveBrowserView plus a BrowserEmptyState variant="connecting" overlay. The overlay dismisses on the neko iframe's first load event, detected via a MutationObserver (the iframe mounts inside LiveBrowserView after the wrapper mounts). LiveBrowserView is untouched. - BrowserSidebar.tsx: replace div[role="option"] with native <button> on SidebarTabRow. Drops the orphan role="option" (no listbox parent), gains keyboard + screen-reader support for free. The close affordance is kept as a role="button" span inside the row to avoid nested <button> elements. Visual styling and collapsed/icon-only state are unchanged.
1 participant
Summary
BrowserSidebar.tsx+browser-ui-store.ts): 220px expanded with tab titles, favicons, and an active-indicator rail; collapses to 44px icon-only. Smooth CSS width transition that respectsprefers-reduced-motionanddata-perf=reduced. State is a lightweight zustand store (not localStorage) so it persists within a session without polluting the session-persistence snapshot.BrowserEmptyState.tsx): new-tab shows a Globe icon + hint instead of a blank white void; the Neko connecting state shows an animated MonitorPlay icon with loading dots.DiscardedPlaceholderrefined to match the design bar's type hierarchy.focus-visiblering.tokens.css: sidebar collapse animation keyframes andbrowser-stream-pulseadded alongside the existingtaos-shimmer/taos-card-enteranimations.Behavior unchanged
LiveBrowserViewiframe and neko_url logic is untouched. Mobile layout is untouched. All sessions/tabs/escalate/mode-toggle behavior is preserved.Test plan
vitest run-- all 10BrowserApp.test.tsxtests passreactnode_modules in worktree)prefers-reduced-motion-- sidebar collapse should be instantCloses #66
Summary by CodeRabbit
Release Notes
New Features
UI & Styling