fix(browser): non-blocking tailscale detect + wire connecting state + sidebar a11y (#124)#1229
Conversation
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? |
|
Warning Review limit reached
More reviews will be available in 11 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThree independent fixes: ChangesFrontend Browser UI
Backend Async Fix
Sequence Diagram(s)sequenceDiagram
participant TabRenderer
participant LiveSessionSlot
participant MutationObserver
participant NekoIframe
TabRenderer->>LiveSessionSlot: render(tabId, nekoUrl, streamToken)
LiveSessionSlot->>LiveSessionSlot: isConnecting=true → show BrowserEmptyState overlay
LiveSessionSlot->>NekoIframe: check if iframe already in DOM
alt iframe already mounted
LiveSessionSlot->>NekoIframe: addEventListener("load", onLoad)
else iframe not yet mounted
LiveSessionSlot->>MutationObserver: observe container
MutationObserver->>NekoIframe: iframe detected in DOM
MutationObserver->>NekoIframe: addEventListener("load", onLoad)
MutationObserver->>MutationObserver: disconnect()
end
NekoIframe-->>LiveSessionSlot: "load" event fires
LiveSessionSlot->>LiveSessionSlot: isConnecting=false → hide overlay
LiveSessionSlot->>NekoIframe: removeEventListener("load", onLoad)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| function LiveSessionSlot({ tabId, nekoUrl, streamToken }: LiveSessionSlotProps) { | ||
| const [connecting, setConnecting] = useState(true); | ||
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| useEffect(() => { | ||
| // Listen for the neko iframe's load event; once it fires, hide the | ||
| // connecting overlay. The iframe is rendered by LiveBrowserView inside | ||
| // wrapperRef, so we can find it once it mounts. | ||
| const wrapper = wrapperRef.current; | ||
| if (!wrapper) return; | ||
|
|
||
| const onLoad = () => setConnecting(false); | ||
|
|
||
| // The iframe may already be present (SSR / fast mount); attach directly if so. | ||
| const iframe = wrapper.querySelector("iframe"); |
There was a problem hiding this comment.
⚠️ Edge Case: Connecting overlay can get stuck if iframe load is missed
LiveSessionSlot keeps the connecting overlay (BrowserEmptyState variant="connecting") visible until the neko iframe fires a load event. The overlay is absolute inset-0 with a solid bg-shell-bg-deep background and no pointer-events-none, so while connecting is true it fully obscures and blocks all interaction with the live session.
There is no fallback if the load event is never observed:
- In the
MutationObserverbranch (lines 395-401), the observer callback runs asynchronously after the iframe is inserted. If the iframe'sloadevent fires before the callback attaches the listener (e.g. cached/instant load), the event is missed andsetConnecting(false)is never called. - If the neko iframe never emits a
loadevent for any reason, the overlay stays up permanently, leaving the user staring at the "Starting…" state over a session that is actually live.
Consider adding a safety timeout that clears connecting after a few seconds regardless, and/or checking iframe.contentDocument?.readyState/iframe.complete-style already-loaded state when attaching the listener so an already-fired load is not missed.
Add a timeout fallback so the connecting overlay can never block the live view indefinitely.:
useEffect(() => {
const wrapper = wrapperRef.current;
if (!wrapper) return;
const onLoad = () => setConnecting(false);
// Safety net: clear the overlay even if the load event is missed.
const timer = window.setTimeout(() => setConnecting(false), 15000);
const attach = (el: HTMLIFrameElement) => el.addEventListener("load", onLoad);
const existing = wrapper.querySelector("iframe");
if (existing) attach(existing);
const observer = new MutationObserver(() => {
const el = wrapper.querySelector("iframe");
if (el) { observer.disconnect(); attach(el); }
});
if (!existing) observer.observe(wrapper, { childList: true, subtree: true });
return () => {
window.clearTimeout(timer);
observer.disconnect();
wrapper.querySelector("iframe")?.removeEventListener("load", onLoad);
};
}, []);
Was this helpful? React with 👍 / 👎
| @@ -218,8 +217,8 @@ function SidebarTabRow({ | |||
| ].join(" ")} | |||
| > | |||
There was a problem hiding this comment.
💡 Quality: Close affordance is a non-focusable span nested in a button
The close affordance was changed from a real <button tabIndex={-1}> to a <span role="button"> to avoid nesting <button> inside the new row <button>. However a <span role="button"> is still interactive content nested inside an interactive <button>, which is invalid per the HTML spec (a button must not contain interactive content) and produces ambiguous semantics for assistive tech. It also has no tabIndex and no keydown handler, so the close action is not keyboard-operable and is announced as a button that can never be reached/activated.
A cleaner structure avoids nesting interactive elements entirely: make the row a non-button container and render the tab-activate target and the close button as sibling interactive elements, or keep the row as a <button> and place the close <button> as a sibling positioned over the row (not a descendant). This restores valid semantics and keyboard access for the close action.
Avoid nested interactive content by making the activate and close controls siblings.:
// Render the row activation and close as sibling buttons instead of nesting:
<div className="group relative flex w-full items-center ...">
<button type="button" aria-label={title} aria-pressed={isActive}
onClick={onActivate} title={title} className="flex flex-1 ...">
{/* favicon + title */}
</button>
{!isPinned && !collapsed && (
<button type="button" aria-label={`Close ${title}`}
onClick={(e) => { e.stopPropagation(); onClose(); }}
className="flex-none ...">
<X size={10} />
</button>
)}
</div>
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
|
| Compact |
|
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 205-211: The span element with role="button" in BrowserSidebar.tsx
that handles the close control is missing keyboard accessibility features. Add
tabIndex={0} to the span to make it focusable for keyboard users, and add an
onKeyDown handler that checks for Enter or Space key presses and calls the
onClose function when detected. This will make the close control accessible to
keyboard users, matching the accessible pattern demonstrated in
SessionBrowser.tsx.
In `@desktop/src/apps/BrowserApp/TabRenderer.tsx`:
- Around line 378-408: The useEffect hook in TabRenderer.tsx has two issues:
first, the empty dependency array means the effect never re-runs when nekoUrl or
streamToken change, so setConnecting(true) is never called when the iframe
reloads for a new session. Second, there's a race condition where the iframe
load event can fire before the listener is attached. To fix this, add nekoUrl
and streamToken to the dependency array and call setConnecting(true) at the
beginning of the effect to reset the connecting state each time it runs.
Additionally, refactor LiveBrowserView to accept an onLoad prop, then pass the
onLoad callback directly to it so the iframe listener can be attached
synchronously during render, eliminating the race condition and MutationObserver
logic entirely.
🪄 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: 1ed32669-976b-46c8-9eb2-8a2b0364408e
📒 Files selected for processing (3)
desktop/src/apps/BrowserApp/BrowserSidebar.tsxdesktop/src/apps/BrowserApp/TabRenderer.tsxtinyagentos/worker/browser_container.py
| useEffect(() => { | ||
| // Listen for the neko iframe's load event; once it fires, hide the | ||
| // connecting overlay. The iframe is rendered by LiveBrowserView inside | ||
| // wrapperRef, so we can find it once it mounts. | ||
| const wrapper = wrapperRef.current; | ||
| if (!wrapper) return; | ||
|
|
||
| const onLoad = () => setConnecting(false); | ||
|
|
||
| // The iframe may already be present (SSR / fast mount); attach directly if so. | ||
| const iframe = wrapper.querySelector("iframe"); | ||
| if (iframe) { | ||
| iframe.addEventListener("load", onLoad); | ||
| return () => iframe.removeEventListener("load", onLoad); | ||
| } | ||
|
|
||
| // Otherwise observe for the iframe to appear, then attach. | ||
| const observer = new MutationObserver(() => { | ||
| const el = wrapper.querySelector("iframe"); | ||
| if (el) { | ||
| observer.disconnect(); | ||
| el.addEventListener("load", onLoad); | ||
| } | ||
| }); | ||
| observer.observe(wrapper, { childList: true, subtree: true }); | ||
| return () => { | ||
| observer.disconnect(); | ||
| const el = wrapper.querySelector("iframe"); | ||
| if (el) el.removeEventListener("load", onLoad); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current listener lifecycle and absence/presence of direct iframe onLoad wiring.
rg -nP --type=tsx 'function LiveSessionSlot|const \[connecting|useEffect\(|\], \[\]\)|setConnecting\(true\)' desktop/src/apps/BrowserApp/TabRenderer.tsx -C3
rg -nP --type=tsx 'interface LiveBrowserViewProps|export function LiveBrowserView|<iframe|onLoad' desktop/src/apps/BrowserApp/LiveBrowserView.tsx -C3Repository: jaylfc/taOS
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Find and examine the TabRenderer.tsx file for the useEffect pattern
cd desktop/src/apps/BrowserApp
echo "=== TabRenderer.tsx useEffect and LiveSessionSlot ==="
cat -n TabRenderer.tsx | sed -n '370,420p'
echo -e "\n=== LiveBrowserView.tsx structure ==="
if [ -f LiveBrowserView.tsx ]; then
cat -n LiveBrowserView.tsx | head -100
else
echo "LiveBrowserView.tsx not found in this directory"
cd ../..
find . -name "LiveBrowserView.tsx" 2>/dev/null
fiRepository: jaylfc/taOS
Length of output: 3271
Effect never re-runs when session URL or token changes, leaving connecting state stale.
The empty dependency array [] means useEffect runs only once at mount. If nekoUrl or streamToken change (e.g., switching to a different live session on the same tab), the iframe reloads but setConnecting(true) is never called again, so the overlay won't show for the new session.
Additionally, there is a narrow race window: if the iframe load event fires between the wrapper check (line 382) and the listener attachment (line 390), the event is lost and connecting stays true indefinitely.
Suggested direction
Add nekoUrl and streamToken to the dependency array, and reset connecting when the effect re-runs. For the timing race, expose an onLoad prop on LiveBrowserView and bind it directly to the iframe element:
function LiveSessionSlot({ tabId, nekoUrl, streamToken }: LiveSessionSlotProps) {
const [connecting, setConnecting] = useState(true);
const wrapperRef = useRef<HTMLDivElement>(null);
useEffect(() => {
+ setConnecting(true);
const wrapper = wrapperRef.current;
if (!wrapper) return;
...
- }, []);
+ }, [nekoUrl, streamToken]);And update LiveBrowserView:
interface LiveBrowserViewProps {
nekoUrl: string;
streamToken: string;
+ onLoad?: () => void;
}
export function LiveBrowserView({ nekoUrl, streamToken, onLoad }: LiveBrowserViewProps) {
const src = `${nekoUrl}`#token`=${streamToken}`;
return (
<iframe
title="Full browser"
src={src}
+ onLoad={onLoad}
sandbox="allow-scripts allow-forms"
...
/>
);
}🤖 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/TabRenderer.tsx` around lines 378 - 408, The
useEffect hook in TabRenderer.tsx has two issues: first, the empty dependency
array means the effect never re-runs when nekoUrl or streamToken change, so
setConnecting(true) is never called when the iframe reloads for a new session.
Second, there's a race condition where the iframe load event can fire before the
listener is attached. To fix this, add nekoUrl and streamToken to the dependency
array and call setConnecting(true) at the beginning of the effect to reset the
connecting state each time it runs. Additionally, refactor LiveBrowserView to
accept an onLoad prop, then pass the onLoad callback directly to it so the
iframe listener can be attached synchronously during render, eliminating the
race condition and MutationObserver logic entirely.
…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.
- TabRenderer: add 15s safety timeout so the 'connecting' overlay can never hang over a live session if the iframe load event is missed (already-loaded before listener attach, or never fires cross-origin). - BrowserSidebar: make the close affordance focusable and keyboard operable (tabIndex + Enter/Space handler). - browser_container: drop the now-misplaced asyncio.to_thread wrap on build_neko_run_args; after #1230 it is pure string assembly. The real blocking call (host DNS resolution) is now offloaded at the route. - browser_sessions route: offload the blocking _connecting_host_ip DNS lookup via asyncio.to_thread so it does not stall the event loop. Note: _detect_tailscale_ip and _resolve_connecting_ip in browser_container.py are now dead code (orphaned by #1230); left in place for a separate cleanup.
jaylfc
force-pushed
the
fix/browser-124-nits
branch
from
d09625c to
f340043
Compare
1 participant
Summary
Async tailscale detect (
tinyagentos/worker/browser_container.py):build_neko_run_args()callsbuild_nat1to1()which runstailscale ipas a blocking subprocess. Wrapped the call inawait asyncio.to_thread()at the async call site inBrowserContainerRunner.start(). The sync function itself is unchanged so all existing tests keep working.Connecting empty state (
desktop/src/apps/BrowserApp/TabRenderer.tsx): IntroducedLiveSessionSlot, a thin wrapper aroundLiveBrowserViewthat showsBrowserEmptyState variant="connecting"as an overlay until the neko iframe fires its firstloadevent (detected viaMutationObserversince the iframe mounts insideLiveBrowserViewafter the wrapper).LiveBrowserViewis untouched.Sidebar a11y (
desktop/src/apps/BrowserApp/BrowserSidebar.tsx): Replaced<div role="option" aria-selected onClick>(orphaned, no listbox parent, no keyboard support) with a native<button>onSidebarTabRow. Keyboard and screen-reader access now work for free. The close affordance usesrole="button"on a<span>to avoid nested<button>elements. Visual styling and collapsed/icon-only state are unchanged.Test results
Closes taOS task #124.
Summary by CodeRabbit
New Features
Improvements