feat(site): desktop panel toolbar, zoom modes, and pop-out window#25585
feat(site): desktop panel toolbar, zoom modes, and pop-out window#25585tracyjohnsonux wants to merge 40 commits into
Conversation
…m, toolbar, and pop-out - Default to 100% native resolution (scaleViewport=false) with scroll-wheel panning for navigating the desktop at full size - Add DesktopToolbar overlay with workspace app icons (left), zoom toggle, pop-out button, and take/release control (right) - Support pop-out to dedicated browser window at /agents/:agentId/desktop with resizeSession=true so remote desktop matches window size - BroadcastChannel coordination between sidebar panel and pop-out - Keyboard shortcuts: Ctrl+0 for fit-to-window, Ctrl+1 for 100% - Toolbar auto-hides to thin strip when controlling desktop - Hook accepts scaleViewport and resizeSession options to avoid direct RFB mutation (React Compiler compatible) - Extended DesktopPanelContext with agent and workspace data for toolbar app icon rendering
…n, 50% pop-out - Make toolbar always visible (remove auto-hide when controlling) - Replace dock-style app icons with Mac-like 'Apps' dropdown menu - Pop-out window defaults to 50% screen size with native (100%) zoom - Toolbar uses compact h-8 height with border-bottom styling
Fetch chat and workspace data in DesktopPopoutPage so the toolbar has access to workspace agent apps for the Apps dropdown menu.
Use capture phase on the wheel handler so it fires before noVNC forwards the event to the remote desktop. This prevents XFCE workspace switching when scrolling to pan the desktop view.
Change toolbar from absolute positioning to flex column layout so it takes its own space above the VNC canvas instead of overlapping the desktop content. Applies to both sidebar panel and pop-out.
… toolbar Command-type apps open terminal tabs when clicked, which is wrong for the desktop context. Only show web apps (those without a command property) in the Apps dropdown.
…pdown Add desktop apps (Chrome) to the toolbar Apps dropdown that launch inside the VNC session via a fire-and-forget terminal connection with DISPLAY=:1 set. Separate from web apps (code-server) which open in browser tabs. Desktop apps list is configurable via the DesktopApp interface.
Use the agent's reconnecting PTY websocket API to execute desktop
app commands with DISPLAY=:1 set. Sends the command as raw text
through /api/v2/workspaceagents/{id}/pty, then closes the socket
after a short delay. No more popup windows.
…iner in fit mode When scale mode is 'fit', enable resizeSession so the remote desktop resolution matches the panel size. Chrome maximized fills exactly the available space. Resizing the panel adjusts the desktop resolution accordingly.
- Sync scaleViewport/resizeSession to RFB via ref mutation, triggered by rfbInstance state changes and option changes - biome-ignore for rfbInstance dep (triggers re-run on connect) - Add overflow-hidden on pop-out VNC container to eliminate right padding from canvas size mismatch
In fit mode: overflow-hidden since resizeSession makes the desktop match the container exactly. In native mode: overflow-scroll with hidden scrollbar styling so the wheel handler can pan without visible scrollbars.
…n container The offscreen div that holds the noVNC canvas was missing overflow:hidden, so the native-resolution canvas caused scrollbars on the inner container.
Always preventDefault+stopPropagation on wheel events in both the sidebar panel and pop-out window. In native mode, translate to scroll panning. In fit mode, just block. Prevents XFCE workspace switching regardless of zoom mode or which view is active.
…n div noVNC internally sets overflow:auto on its screen container div (rfb.js line 236), causing scrollbars. Override it to hidden immediately after RFB creation.
Remove resizeSession option since portabledesktop hardcodes 1920x1080 and the VNC server does not honor RFB resize requests. Remove overflow:hidden hacks that were clipping content instead of actually resizing. Restore clean two-mode approach: - Fit mode: scaleViewport shrinks desktop to fit, overflow hidden - Native mode: full resolution, hidden scrollbars, wheel panning
Remove wheel event capture, scrollRef, hidden-scrollbar styling, and VncContainer component. The scroll-wheel panning approach does not work well with the fixed 1920x1080 VNC framebuffer. Both modes now use simple overflow:hidden containers.
The reconnecting PTY expects JSON messages ({data: '...'}), not
raw text bytes. Send the launch command as a JSON-encoded
ReconnectingPTYRequest so the agent actually executes it.
…launcher Remove the Apps dropdown, DesktopApp interface, PTY launcher, agent/workspace data fetching in pop-out, and all related plumbing. Desktop apps are better handled as native XFCE desktop shortcuts configured in the template startup script. The toolbar now contains only zoom toggle, pop-out, and control buttons.
…le style Move Take/Release control to left side of toolbar. Use subtle variant (no outline) for both states. Rename to 'Take control' and 'Release control' with full labels.
Scale viewport to fit the container by default so the full desktop is visible without scrolling. Users can toggle to native resolution with the zoom button or Ctrl+1.
Replace icon-only zoom button with labeled buttons: - Scaling icon + 'Zoom to fit' (when in native mode) - Maximize icon + 'Zoom to 100%' (when in fit mode)
Use bg-surface-primary for the VNC container background so the letterbox bars in fit-to-window mode match the chat message boxes in both light and dark themes.
noVNC hardcodes rgb(40,40,40) as its screen background, which creates a visible mismatch with the app surface colors. Read the --surface-primary CSS variable at connect time and apply it to the RFB background property so the letterbox margins match the chat message boxes in both light and dark mode.
Read the CSS variable into a local before setting rfb.background. The previous code set an invalid value first, then read back the getter (which returns the old screen style), producing a corrupted hsl() string.
The chat message cards use bg-surface-secondary, which sits on top of the surface-primary page background. Match the desktop letterbox margins to surface-secondary so they blend with the message cards.
…lContext These were added for a planned apps dropdown that was removed from scope. No consumer reads them.
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero). This is a mechanical first pass only; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
The PR cleanly separates toolbar, panel, and pop-out page concerns, and the BroadcastChannel coordination pattern is well-chosen for cross-window state sync.
4 P2, 1 P3, 1 Nit. The P2s cluster around duplication (three copies of ScaleMode, two copies of CHANNEL_PREFIX, two copies of the keyboard shortcut handler) and missing Storybook coverage for 222 lines of new component code.
"Export from a shared location." Netero, tapping his finger on the duplicated constants
site/src/pages/AgentsPage/components/RightPanel/DesktopPanel.stories.tsx:6
Nit [DEREM-7] defaults omits onPopOut, so no story ever renders the Detach button. Adding a Connected variant with onPopOut: fn() would cover that branch. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
- Extract ScaleMode type and CHANNEL_PREFIX to shared desktopConstants.ts (DEREM-1, DEREM-2) - Extract zoom keyboard shortcuts to useZoomShortcuts hook (DEREM-3) - Add DesktopToolbar Storybook stories for all states (DEREM-4) - Add onPopOut to DesktopPanel story defaults (DEREM-7) - Fix stale scaleViewport comment in useDesktopConnection (DEREM-6)
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 is blocked. 5 of 7 findings from round 1 were addressed in 137f439 (shared constants file, extracted hook, new toolbar stories, comment fix). Good progress.
However, DEREM-5 (no Storybook story for DesktopPopoutPage, a 122-line page component with four status branches and BroadcastChannel lifecycle) received no response and no fix. The commit message references DEREM-1/2/3/4/6/7 but not DEREM-5.
Further review is blocked until the author either adds a story for DesktopPopoutPage or responds to explain why one is not needed. The full review panel will run once all first-pass findings are addressed.
🤖 This review was automatically generated with Coder Agents.
Remove querySelector-based overflow override from try/catch (breaks React Compiler). Use Tailwind child selector instead. Also removes the screenEl code that was accidentally included in the prior commit.
Extract DesktopPopoutPageView from the route component so all four status branches (connecting, connected, error, disconnected) can be covered by Storybook stories without needing router or WebSocket infrastructure.
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review, round 3 (Netero). All 7 findings from round 1 are confirmed fixed. Good work consolidating shared types, constants, and the zoom shortcut hook.
2 new P2, 1 P3. The P2s are a 3.6MB unreferenced PNG that permanently inflates repo history, and the same class of type-duplication bug that DEREM-2 addressed (this time DesktopConnectionStatus in three files).
This is a mechanical first pass only; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
"Same class of bug, different type alias." Netero, shaking his head
site/static/Coder_Background_Dark.png:1
P2 [DEREM-8] This 3.6MB PNG has zero references anywhere in the codebase. grep -rn 'Coder_Background' across the entire tree returns nothing. No component, story, HTML template, or config imports it. Dead on arrival, and at 3.6MB it permanently inflates repository history. Remove it from the commit. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
- Change role='toolbar' to role='group' (toolbar requires arrow-key navigation we don't implement) - Add aria-pressed to Take/Release control toggle button - Replace pointer-events-none with inert attribute on VNC container when not controlling, with role='application' and descriptive aria-label for screen readers - Add role='status' to popped-out placeholder for screen reader announcement - Remove unused cn import from DesktopPopoutPage
…fault Tests assumed scaleViewport defaulted to true at RFB creation. The hook now inits to false and syncs via a secondary effect, so tests that verify scaleViewport=true must pass it explicitly.
- Export DesktopConnectionStatus from desktopConstants.ts, import in all consumers instead of defining locally (DEREM-9) - Add play functions to DesktopToolbar and DesktopPopoutPage stories to verify button callbacks and status-conditional rendering (DEREM-10)
|
/coder-agents-review |
The ResizeObserver callback re-assigns the current scaleViewport value to trigger a noVNC rescale. It does not force a specific value. Test now verifies the setter was called rather than checking for a specific boolean.
There was a problem hiding this comment.
Panel review, round 4 (Bisky, Hisoka, Mafuuu, Mafu-san, Pariston, Nami, Meruem, Gon, Leorio). All 10 prior Netero findings confirmed fixed. The shared constants extraction, zoom shortcut hook, and story coverage are all clean.
The toolbar redesign, zoom modes, and pop-out architecture are well-structured. Pariston systematically challenged four framings of the problem and found no misalignment; the BroadcastChannel pattern is the simplest viable coordination mechanism, and the toolbar solves real discoverability issues.
1 P2, 6 P3, 2 Nit. The P2 is a convergent finding from 4 reviewers: the BroadcastChannel protocol has no discovery handshake, so parent navigation drops coordination state and creates dual VNC sessions.
Process observations (Mafu-san): The commit history shows a speculative development pattern (CSS override committed untested, reverted 117s later; an entire desktop launcher feature built across 8 commits then fully removed in 1). The unreferenced 3.6MB PNG (DEREM-8, now fixed) was introduced during a review-fix commit. These are workflow signals, not blocking code defects.
"The pop-out sends
popout-openedexactly once. The parent's listener is registered in auseEffect. If the user navigates away,DesktopPanelunmounts, the channel closes. When the user navigates back, a freshDesktopPanelmounts withisPoppedOut = false. The protocol has no discovery mechanism." Mafuuu, tracing the lifecycle
🤖 This review was automatically generated with Coder Agents.
- Move misplaced DesktopConnectionStatus import to top (DEREM-11) - Add BroadcastChannel retry for pop-out race condition (DEREM-13) - Move desktop route inside RequireAuth (DEREM-14) - Add scaleViewport toggle test (DEREM-16) - Match error text between panel and popout views (DEREM-18) - Remove redundant sync comment (DEREM-19)
|
/coder-agents-review |
There was a problem hiding this comment.
Round 5 is blocked. Strong progress: 6 of 9 open findings addressed in 6d0fb13 (BroadcastChannel handshake retry, route moved inside RequireAuth, scaleViewport toggle test, improved error message, import fix, comment removal). DEREM-12 and DEREM-17 acknowledged with reasonable author explanations.
However, DEREM-15 (zoom shortcuts fire simultaneously with noVNC key forwarding when controlling the remote desktop) was deferred with "Noted for follow-up" but no linked ticket or issue. The author acknowledged the concern is valid and suggested suppressing shortcuts while isControlling is true.
This needs a human decision: fix in this PR, file a GitHub issue to track the follow-up, or explicitly accept that Ctrl+0/Ctrl+1 will fire both locally and on the remote desktop when controlling. The review panel cannot accept a known gap as permanent.
🤖 This review was automatically generated with Coder Agents.
Redesigns the agent desktop panel with a persistent toolbar, zoom modes, and a detachable pop-out window.
Changes
Toolbar (
DesktopToolbar)subtlevariant with icon + labelh-8height,bg-surface-primarybackground with bottom borderZoom modes
scaleViewport = true) so the full 1920x1080 desktop is visibleCtrl+0fit,Ctrl+1native)rgb(40,40,40)to--surface-secondaryso letterbox margins match the app theme in light and dark modePop-out window
/agents/:agentId/desktopfor a dedicated desktop windowOther
useDesktopConnectionhook acceptsscaleViewportoption, synced to the RFB instance via a secondary effectDesktopPanelContextextended withagentandworkspacefields