Add embedded VNC viewer - #643
Open
lixmal wants to merge 6 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVNC remote access now uses noVNC over a NetBird WebSocket proxy. The change adds VNC controls, session lifecycle handling, query restoration, cursor and clipboard support, and explicit IPv4/IPv6 selection for SSH and RDP. ChangesVNC Remote Access Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VNCButton
participant VNCPage
participant NetBird
participant useVNC
participant noVNC
User->>VNCButton: Open VNC
VNCButton->>VNCPage: Open peer VNC page
VNCPage->>NetBird: Establish temporary tunnel
VNCPage->>useVNC: Start VNC connection
useVNC->>NetBird: Create VNC proxy
NetBird-->>useVNC: Return proxy URL
useVNC->>noVNC: Initialize RFB
noVNC-->>VNCPage: Report connection status
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/modules/remote-access/vnc/useVNCQueryParams.ts (1)
59-59: 💤 Low valueUse the
useLocalStoragehook consistently.Line 59 directly calls
localStorage.getItem, but the hook already imports and usesuseLocalStorage(line 35). For consistency and testability, prefer using the hook's getter or a ref pattern.🤖 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 `@src/modules/remote-access/vnc/useVNCQueryParams.ts` at line 59, The line directly calling localStorage.getItem should be replaced to use the already-imported useLocalStorage hook for consistency and testability: inside the useVNCQueryParams function, obtain the stored value via the hook's API (e.g., call useLocalStorage('netbird-query-params') or use the hook's provided getter/ref) instead of localStorage.getItem and assign that result to the storedParams variable; update any subsequent logic that reads storedParams to use the hook-returned value.src/modules/remote-access/vnc/VNCButton.tsx (1)
33-39: ⚡ Quick winConsider handling popup blocker scenarios.
window.opencan returnnullif blocked by popup blockers. Consider checking the return value and notifying the user if the window fails to open.💡 Example implementation
const openVNCPage = () => { - window.open( + const newWindow = window.open( `/peer/vnc?id=${peer.id}`, "_blank", "noopener,noreferrer,width=1200,height=800,left=100,top=100,location=no,toolbar=no,menubar=no,status=no", ); + if (!newWindow) { + notify({ + title: "Popup Blocked", + description: "Please allow popups for this site to open VNC sessions.", + type: "error", + }); + } };🤖 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 `@src/modules/remote-access/vnc/VNCButton.tsx` around lines 33 - 39, openVNCPage uses window.open to open the VNC page but doesn't handle popup blockers; modify the openVNCPage function to capture the return value of window.open (called with `peer.id`) and check for null/undefined, and if it failed, surface a user-facing notification (e.g., alert/toast via the app's notification system) and optionally log the failure with context including the peer.id so users/developers know the popup was blocked.src/modules/remote-access/vnc/useVNC.ts (1)
274-279: ⚡ Quick winAvoid relying on noVNC's private
_updateScaleAPI.The
_updateScalemethod is internal to noVNC and undocumented; calling it directly risks breakage on upgrades. According to noVNC's public API, viewport scaling should be handled automatically whenscaleViewportistrue. If resizing doesn't rescale the viewport correctly, first ensure the VNC container has explicit height/width set. If the issue persists, consider reporting it as a bug to the noVNC project rather than working around it with private APIs.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 274 - 279, The handleResize debounce currently calls noVNC's private _updateScale on rfbRef.current which is unsafe; remove the direct call to rfbRef.current._updateScale and rely on the public behavior when rfbRef.current?.scaleViewport is true. Instead ensure the VNC container/component that handleResize acts on has explicit width/height in CSS or inline style so noVNC can recalculate scale automatically; if automatic scaling still fails, revert the workaround and open an upstream issue with the noVNC project. Update the handleResize function and any related resize logic (rfbRef, scaleViewport, and the VNC container setup) accordingly.
🤖 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 `@package.json`:
- Line 21: The package.json currently pins the pre-release dependency string
"`@novnc/novnc`": "^1.7.0" — replace that version with the stable release "^1.6.0"
in package.json (update the dependency entry for "`@novnc/novnc`"), then run your
package manager (npm/yarn/pnpm) to update node_modules and the lockfile so the
lockfile reflects the new stable version.
In `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Line 31: The page currently calls useVNCQueryParams and destructures peerId
which, if missing, keeps fetch disabled and leaves FullScreenLoading forever;
update the VNC page component to explicitly detect when peerId is falsy and
handle it (e.g., render an error/validation UI or redirect) instead of rendering
FullScreenLoading with no recovery; locate useVNCQueryParams usage and the
FullScreenLoading render path in the VNC page (references: peerId,
useVNCQueryParams, FullScreenLoading) and add a clear early-return branch that
stops any data fetching and shows the error state so the user can correct or
navigate away (apply same change to the similar logic around lines 40-50).
- Around line 144-147: The catch handler for vnc.connect(...) currently calls
sendErrorNotification and also sets setConnectFailed, which causes the separate
vnc.error effect to emit the same toast; update the logic so only one
notification is sent: either remove sendErrorNotification from the .catch(...)
and rely on the vnc.error effect, or add a short-lived guard flag (e.g., a ref
like suppressVncErrorToast) checked in both places so .catch(...) sets the flag
before calling setConnectFailed and the vnc.error effect skips sending if the
flag is set; apply the same change to the other occurrence around the block that
starts at the second location (lines ~177-180) to prevent duplicate toasts.
In `@src/modules/remote-access/vnc/useVNCQueryParams.ts`:
- Around line 58-81: The restoration logic in useVNCQueryParams currently only
preserves "id" (storedPeerId) and resets everything else to defaultSettings;
instead, parse urlParams (from paramsString) and copy all relevant keys into
newSearchParams and the state passed to setParams so mode, username, quality,
scale, resize, cursor (and id -> peerId) are restored if present, falling back
to defaultSettings for any missing values; then call router.replace with the
full reconstructed query string, call setLocalQueryParams("") (or clear
localStorage) and ensure setParams receives the merged object (e.g., { peerId:
urlParams.get("id"), mode: urlParams.get("mode") || defaultSettings.mode,
username: urlParams.get("username") || "", settings: {
...defaultSettings.settings, quality: urlParams.get("quality") ||
defaultSettings.settings.quality, scale: urlParams.get("scale") ||
defaultSettings.settings.scale, resize: urlParams.get("resize") ||
defaultSettings.settings.resize, cursor: urlParams.get("cursor") ||
defaultSettings.settings.cursor } }) so restored values persist after the auth
redirect.
In `@src/modules/remote-access/vnc/VNCButton.tsx`:
- Around line 33-39: The openVNCPage handler uses a relative URL which can
resolve incorrectly; update the URL in openVNCPage to an absolute path by
prefixing with a slash (e.g., "/peer/vnc?id=" + peer.id or template
`/peer/vnc?id=${peer.id}`) when calling window.open so it always navigates to
the correct route; keep the existing window.open options (target "_blank" and
window features) unchanged.
In `@src/modules/remote-access/vnc/VNCToolbar.tsx`:
- Around line 36-39: The cleanup function currently only removes event listeners
but doesn't reset drag state; update the unmount cleanup in VNCToolbar to also
set draggingRef.current = false and restore document.body.style.userSelect = ""
(or its previous value) so a mid-drag unmount doesn't leave selection disabled;
locate the effect that adds window.addEventListener for "pointermove" and
"pointerup" (handlers onMove and onUp) and extend its return to reset
draggingRef and userSelect alongside removing the listeners.
In `@src/modules/remote-access/vnc/websocket-proxy.ts`:
- Around line 167-171: The construct trap for the WebSocket proxy assumes
args[0] is a string and calls .includes(), which will throw if a URL object is
passed; update the construct function so it normalizes the first argument: if
args[0] is a URL instance use args[0].toString() or args[0].href, otherwise
coerce it to a string, then perform the includes("vnc.proxy.local") check and
return new VNCProxyWebSocket(urlString) cast as WebSocket when matched; keep
existing behavior for non-matching values.
---
Nitpick comments:
In `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 274-279: The handleResize debounce currently calls noVNC's private
_updateScale on rfbRef.current which is unsafe; remove the direct call to
rfbRef.current._updateScale and rely on the public behavior when
rfbRef.current?.scaleViewport is true. Instead ensure the VNC
container/component that handleResize acts on has explicit width/height in CSS
or inline style so noVNC can recalculate scale automatically; if automatic
scaling still fails, revert the workaround and open an upstream issue with the
noVNC project. Update the handleResize function and any related resize logic
(rfbRef, scaleViewport, and the VNC container setup) accordingly.
In `@src/modules/remote-access/vnc/useVNCQueryParams.ts`:
- Line 59: The line directly calling localStorage.getItem should be replaced to
use the already-imported useLocalStorage hook for consistency and testability:
inside the useVNCQueryParams function, obtain the stored value via the hook's
API (e.g., call useLocalStorage('netbird-query-params') or use the hook's
provided getter/ref) instead of localStorage.getItem and assign that result to
the storedParams variable; update any subsequent logic that reads storedParams
to use the hook-returned value.
In `@src/modules/remote-access/vnc/VNCButton.tsx`:
- Around line 33-39: openVNCPage uses window.open to open the VNC page but
doesn't handle popup blockers; modify the openVNCPage function to capture the
return value of window.open (called with `peer.id`) and check for
null/undefined, and if it failed, surface a user-facing notification (e.g.,
alert/toast via the app's notification system) and optionally log the failure
with context including the peer.id so users/developers know the popup was
blocked.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 479650dc-fc85-49ef-95b5-53bfe9522fda
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/interfaces/Peer.tssrc/modules/peers/PeerConnectButton.tsxsrc/modules/remote-access/vnc/VNCButton.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsxsrc/modules/remote-access/vnc/VNCTooltip.tsxsrc/modules/remote-access/vnc/novnc.d.tssrc/modules/remote-access/vnc/useVNC.tssrc/modules/remote-access/vnc/useVNCQueryParams.tssrc/modules/remote-access/vnc/websocket-proxy.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/modules/remote-access/vnc/useVNCQueryParams.ts`:
- Line 57: The mode query param is being force-cast without validating its value
in useVNCQueryParams (variable mode and the second occurrence on line 91);
replace the direct type assertion with a validation step: read the raw string
from searchParams.get("mode"), check it against an allowlist like
["attach","session"] (e.g., allowedModes.includes(raw)), and only then
assign/cast to "attach" | "session", otherwise fall back to "attach"; apply the
same validation logic to the other occurrence on line 91 so arbitrary strings
cannot be treated as valid modes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e375997-c69e-499b-8f68-a1a14d1df03f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/interfaces/Peer.tssrc/modules/peers/PeerConnectButton.tsxsrc/modules/remote-access/vnc/VNCButton.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsxsrc/modules/remote-access/vnc/VNCTooltip.tsxsrc/modules/remote-access/vnc/novnc.d.tssrc/modules/remote-access/vnc/useVNC.tssrc/modules/remote-access/vnc/useVNCQueryParams.tssrc/modules/remote-access/vnc/websocket-proxy.ts
✅ Files skipped from review due to trivial changes (3)
- src/modules/remote-access/vnc/novnc.d.ts
- src/auth/SecureProvider.tsx
- src/app/(dashboard)/peer/page.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- package.json
- src/app/globals.css
- src/interfaces/Peer.ts
- src/modules/peers/PeerConnectButton.tsx
- src/modules/remote-access/vnc/VNCTooltip.tsx
- src/modules/remote-access/vnc/VNCToolbar.tsx
- src/modules/remote-access/vnc/VNCButton.tsx
- src/app/(remote-access)/peer/vnc/page.tsx
- src/modules/remote-access/vnc/websocket-proxy.ts
- src/modules/remote-access/vnc/useVNC.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/(remote-access)/peer/vnc/page.tsx (1)
164-177: 💤 Low valueRemove unused
sendErrorNotificationfrom dependency array.
sendErrorNotificationis included in the effect's dependency array but is never called within the effect body. Removing it clarifies the effect's actual dependencies.♻️ Suggested fix
], [ client.status, vnc.connect, vnc.status, peer.ip, isNetBirdConnecting, showSetup, mode, username, accessToken, settings, connectFailed, - sendErrorNotification, ]);🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx around lines 164 - 177, Remove sendErrorNotification from the useEffect dependency array in the VNC page (the effect that currently lists client.status, vnc.connect, vnc.status, peer.ip, isNetBirdConnecting, showSetup, mode, username, accessToken, settings, connectFailed, sendErrorNotification). Edit the effect where sendErrorNotification is not referenced in the body and delete it from the dependencies so the array only includes actual used symbols (client.status, vnc.connect, vnc.status, peer.ip, isNetBirdConnecting, showSetup, mode, username, accessToken, settings, connectFailed).
🤖 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.
Nitpick comments:
In `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 164-177: Remove sendErrorNotification from the useEffect
dependency array in the VNC page (the effect that currently lists client.status,
vnc.connect, vnc.status, peer.ip, isNetBirdConnecting, showSetup, mode,
username, accessToken, settings, connectFailed, sendErrorNotification). Edit the
effect where sendErrorNotification is not referenced in the body and delete it
from the dependencies so the array only includes actual used symbols
(client.status, vnc.connect, vnc.status, peer.ip, isNetBirdConnecting,
showSetup, mode, username, accessToken, settings, connectFailed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69b6175f-8535-4c25-a72d-23ff7fafb2b0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/interfaces/Peer.tssrc/modules/peers/PeerConnectButton.tsxsrc/modules/remote-access/vnc/VNCButton.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsxsrc/modules/remote-access/vnc/VNCTooltip.tsxsrc/modules/remote-access/vnc/novnc.d.tssrc/modules/remote-access/vnc/useVNC.tssrc/modules/remote-access/vnc/useVNCQueryParams.tssrc/modules/remote-access/vnc/websocket-proxy.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/globals.css
🚧 Files skipped from review as they are similar to previous changes (7)
- src/interfaces/Peer.ts
- src/app/(dashboard)/peer/page.tsx
- src/modules/remote-access/vnc/novnc.d.ts
- package.json
- src/modules/remote-access/vnc/VNCButton.tsx
- src/modules/remote-access/vnc/useVNC.ts
- src/modules/remote-access/vnc/VNCToolbar.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/modules/remote-access/useNetBirdClient.ts (1)
303-316: ⚡ Quick winGuard
netbirdGenerateVNCSessionKey()against throwing.
wasmGenerate()is called outside a dedicated try/catch within the widertry. If the WASM helper throws (e.g., key generation fails inside Go), the entireconnectTemporaryflow aborts before the WireGuardconnect()call, even though falling back to a sessionless connect is the documented behavior for older WASM bundles. Wrap the call so a throwing helper degrades tosessionPub = undefinedand is logged, rather than failing the temporary-access request.🛡️ Proposed fix
const wasmGenerate = (window as any).netbirdGenerateVNCSessionKey; if (typeof wasmGenerate === "function") { - const sk = wasmGenerate(); - if (sk && typeof sk === "object" && typeof sk.publicKey === "string") { - sessionPub = sk.publicKey; - keySessionId = sk.sessionId ?? null; + try { + const sk = wasmGenerate(); + if (sk && typeof sk === "object" && typeof sk.publicKey === "string") { + sessionPub = sk.publicKey; + keySessionId = sk.sessionId ?? null; + } + } catch (e) { + console.warn("netbirdGenerateVNCSessionKey failed:", e); } }🤖 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 `@src/modules/remote-access/useNetBirdClient.ts` around lines 303 - 316, Wrap the call to the WASM helper (the wasmGenerate / netbirdGenerateVNCSessionKey invocation inside connectTemporary) in a try/catch so any thrown errors are caught and do not abort the flow; on error set sessionPub = undefined and keySessionId = null, log the exception (with context) and continue so the fallback sessionless connect path can proceed. Ensure you only change the block that calls wasmGenerate() and preserve the existing type checks (sk.publicKey) and behavior when the helper returns normally.src/modules/remote-access/vnc/useVNC.ts (1)
334-350: 💤 Low valueStyle nit: short-circuit expression statement on Line 341.
rfbRef.current?.scaleViewport && rfbRef.current._updateScale?.();works at runtime, but it's an expression statement using&&as a guard, which is awkward to read and is typically flagged by@typescript-eslint/no-unused-expressions. Consider a straightforwardif:♻️ Proposed refactor
- timeout = setTimeout(() => { - rfbRef.current?.scaleViewport && rfbRef.current._updateScale?.(); - }, 200); + timeout = setTimeout(() => { + if (rfbRef.current?.scaleViewport) { + rfbRef.current._updateScale?.(); + } + }, 200);🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 334 - 350, The short-circuit expression in the handleResize callback uses "rfbRef.current?.scaleViewport && rfbRef.current._updateScale?.();" which is an unused-expression style and should be replaced with an explicit conditional; update the handleResize function inside the useEffect to first safely grab rfbRef.current, then if rfbRef.current?.scaleViewport is truthy call rfbRef.current._updateScale?.(), so replace the && guard with a clear if-statement referencing rfbRef and _updateScale to satisfy linters and improve readability.
🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 37-54: The current render logic can get stuck on FullScreenLoading
when peer fetch fails because peerId is true, peer is undefined and
isPeerLoading is false; update the JSX conditional in the component rendering
(the block that currently chooses between VNCSession, MissingPeerError, and
FullScreenLoading) to explicitly handle the case peerId && !peer &&
!isPeerLoading and render an error state (reuse MissingPeerError or add a new
PeerLoadError component) so failed fetches surface a "peer not found / failed to
load" message instead of an infinite loading spinner; ensure you reference the
same variables (peerId, peer, isPeerLoading, ready) and components (VNCSession,
MissingPeerError, FullScreenLoading) when adjusting the conditional.
- Around line 114-139: The inline comment above the guards in connectNetBird is
misleading: connectTemporary returns a TemporaryConnectResult object on all
paths (with fields possibly null), not null itself; update the comment to state
that the returned object's fields (e.g., targetPubKey and keySessionId) may be
null and that the existing if (result.targetPubKey) / if (result.keySessionId)
checks intentionally avoid overwriting earlier valid values. Reference
connectNetBird, connectTemporary, TemporaryConnectResult, result.targetPubKey
and result.keySessionId when making the change.
In `@src/modules/remote-access/useNetBirdClient.ts`:
- Around line 280-290: The early-return in connectTemporary (checking
netBirdStore.getState().status against NetBirdStatus.CONNECTING/CONNECTED)
currently returns targetPubKey: null and keySessionId: null, which loses
per-VNC-session X25519 metadata; change this path to either (A) remint/refetch
the VNC session key before returning by invoking the same helper used in the
fresh-connect flow so targetPubKey and keySessionId are populated, or (B) return
a discriminated result indicating a reused tunnel (e.g., add a reusedTunnel:
true flag and preserve any existing session metadata from netBirdStore or the
netBird client) so callers can distinguish "reused tunnel" vs "fresh connect"
and act accordingly; update connectTemporary to use one of these approaches and
adjust any callers that depend on targetPubKey/keySessionId (references:
connectTemporary, netBirdStore, NetBirdStatus, targetPubKey, keySessionId).
---
Nitpick comments:
In `@src/modules/remote-access/useNetBirdClient.ts`:
- Around line 303-316: Wrap the call to the WASM helper (the wasmGenerate /
netbirdGenerateVNCSessionKey invocation inside connectTemporary) in a try/catch
so any thrown errors are caught and do not abort the flow; on error set
sessionPub = undefined and keySessionId = null, log the exception (with context)
and continue so the fallback sessionless connect path can proceed. Ensure you
only change the block that calls wasmGenerate() and preserve the existing type
checks (sk.publicKey) and behavior when the helper returns normally.
In `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 334-350: The short-circuit expression in the handleResize callback
uses "rfbRef.current?.scaleViewport && rfbRef.current._updateScale?.();" which
is an unused-expression style and should be replaced with an explicit
conditional; update the handleResize function inside the useEffect to first
safely grab rfbRef.current, then if rfbRef.current?.scaleViewport is truthy call
rfbRef.current._updateScale?.(), so replace the && guard with a clear
if-statement referencing rfbRef and _updateScale to satisfy linters and improve
readability.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b67cc0b-2da6-4287-a782-638af298d0fd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/interfaces/Peer.tssrc/modules/peers/PeerConnectButton.tsxsrc/modules/remote-access/useNetBirdClient.tssrc/modules/remote-access/vnc/VNCButton.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsxsrc/modules/remote-access/vnc/VNCTooltip.tsxsrc/modules/remote-access/vnc/novnc.d.tssrc/modules/remote-access/vnc/useVNC.tssrc/modules/remote-access/vnc/useVNCQueryParams.tssrc/modules/remote-access/vnc/websocket-proxy.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/remote-access/useNetBirdClient.ts (1)
338-340:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
initializein dependency array.The callback uses
await initialize()at line 306, butinitializeis not listed in the dependency array. This could cause stale closure issues ifinitializechanges identity.🛡️ Proposed fix
- [connect, peerRequest], + [connect, initialize, peerRequest],🤖 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 `@src/modules/remote-access/useNetBirdClient.ts` around lines 338 - 340, The useCallback that depends on [connect, peerRequest] also calls await initialize(), which can lead to stale closures; update the dependency array for the callback to include initialize (so it becomes [connect, peerRequest, initialize]) to ensure the callback is recreated when initialize's identity changes; locate the callback in useNetBirdClient (the function that references connect, peerRequest and calls initialize) and add initialize to its dependency list.
♻️ Duplicate comments (1)
src/app/(remote-access)/peer/vnc/page.tsx (1)
37-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRender a failure state when peer fetch settles without data.
When
peerIdis present but the fetch resolves with nopeer, this branch still falls through toFullScreenLoadingindefinitely.🔧 Suggested fix
const { data: peer, isLoading: isPeerLoading, + error: peerError, } = useFetchApi<Peer>(`/peers/${peerId}`, true, false, !!peerId); @@ - ) : ready && !peerId ? ( + ) : ready && (!peerId || (!!peerId && !peer && !isPeerLoading)) ? ( <MissingPeerError /> + ) : peerError ? ( + <MissingPeerError /> ) : ( <FullScreenLoading /> )}🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx around lines 37 - 51, The current JSX conditional falls back to FullScreenLoading when peerId is set but the fetch finished without returning peer; update the render logic in the component that returns the VNCSession block so you explicitly handle the "peerId && !isPeerLoading && !peer" case and render a failure state (e.g., MissingPeerError or a new PeerNotFound component) instead of FullScreenLoading; locate the existing conditional that references peerId, peer, isPeerLoading, ready and VNCSession and insert a branch before the final loading fallback that returns the failure component when the fetch has settled with no peer.
🧹 Nitpick comments (1)
src/modules/remote-access/vnc/novnc.d.ts (1)
1-46: noVNC RFB typings are largely consistent with the actual API surface
RFBOptionsfieldscredentials,repeaterID, andwsProtocolsmatch noVNC’s documented RFB options, and the internal scaling behavior behindscaleViewportcorresponds toRFB.prototype._updateScale.- Declared RFB methods (
sendCredentials,sendCtrlAltDel,clipboardPasteFrom,machineShutdown/machineReboot/machineReset, etc.) match the documented noVNC RFB API.- Optional refactor:
_updateScale?()is internal/underscore-prefixed and isn’t referenced anywhere in the repo codebase; consider removing it fromsrc/modules/remote-access/vnc/novnc.d.ts(line ~44) unless you explicitly depend on it.🤖 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 `@src/modules/remote-access/vnc/novnc.d.ts` around lines 1 - 46, The declared optional internal method _updateScale?() on the RFB class in src/modules/remote-access/vnc/novnc.d.ts is an underscore-prefixed internal API and appears unused; remove the _updateScale?() declaration from the RFB interface in this file (or, if you do actually rely on it, leave it but add a comment explaining why) — search for usages of _updateScale in the repo to confirm it’s unused, then delete the _updateScale?() line from the RFB class declaration in novnc.d.ts.
🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 131-138: The current catch/finally resets isNetBirdConnecting
immediately which retriggers the effect that calls connectTemporary and causes
rapid retry/toast loops; modify the logic around
connectTemporary/isNetBirdConnecting by adding a cooldown or failure flag (e.g.,
netBirdRetryDisabled or lastNetBirdAttempt timestamp) and check that
flag/timestamp in the effect that invokes connectTemporary to skip retries until
a configurable backoff period has elapsed or until user action; set the flag (or
record timestamp) inside the catch block when sendErrorNotification is called
and only clear it after the backoff expires (or when user retries), ensuring
setIsNetBirdConnecting(false) does not alone cause an immediate re-attempt.
In `@src/auth/SecureProvider.tsx`:
- Around line 32-50: The effect is persisting query params for unauthenticated
users on any path; restrict persistence to preserve routes only by using the
existing PRESERVE_QUERY_PARAMS_PATHS/currentPath check: only run the query-param
capture logic when onPreservePath is true (and the user is unauthenticated),
e.g. move the try/URLSearchParams/VALID_PARAMS/QUERY_PARAMS_KEY block behind an
if(onPreservePath) (or return early when not onPreservePath) so only routes in
PRESERVE_QUERY_PARAMS_PATHS can store params.
In `@src/modules/remote-access/vnc/VNCToolbar.tsx`:
- Around line 21-26: VNCToolbar's localStorage reads/writes (STORAGE_KEY) can
throw in restricted/private storage contexts; wrap both the initial getter in
the useState initializer for xPercent and any subsequent localStorage.setItem
calls (e.g., where setXPercent persists) in safe guards: check typeof window !==
"undefined" and wrap getItem/setItem in try/catch, falling back to default 50 on
read errors and silently skipping writes on errors, so the component continues
to function even if localStorage is unavailable.
---
Outside diff comments:
In `@src/modules/remote-access/useNetBirdClient.ts`:
- Around line 338-340: The useCallback that depends on [connect, peerRequest]
also calls await initialize(), which can lead to stale closures; update the
dependency array for the callback to include initialize (so it becomes [connect,
peerRequest, initialize]) to ensure the callback is recreated when initialize's
identity changes; locate the callback in useNetBirdClient (the function that
references connect, peerRequest and calls initialize) and add initialize to its
dependency list.
---
Duplicate comments:
In `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 37-51: The current JSX conditional falls back to FullScreenLoading
when peerId is set but the fetch finished without returning peer; update the
render logic in the component that returns the VNCSession block so you
explicitly handle the "peerId && !isPeerLoading && !peer" case and render a
failure state (e.g., MissingPeerError or a new PeerNotFound component) instead
of FullScreenLoading; locate the existing conditional that references peerId,
peer, isPeerLoading, ready and VNCSession and insert a branch before the final
loading fallback that returns the failure component when the fetch has settled
with no peer.
---
Nitpick comments:
In `@src/modules/remote-access/vnc/novnc.d.ts`:
- Around line 1-46: The declared optional internal method _updateScale?() on the
RFB class in src/modules/remote-access/vnc/novnc.d.ts is an underscore-prefixed
internal API and appears unused; remove the _updateScale?() declaration from the
RFB interface in this file (or, if you do actually rely on it, leave it but add
a comment explaining why) — search for usages of _updateScale in the repo to
confirm it’s unused, then delete the _updateScale?() line from the RFB class
declaration in novnc.d.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2164f85b-7db4-434c-81e2-b92183be16ee
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/components/ui/FullScreenLoading.tsxsrc/interfaces/Peer.tssrc/modules/peers/PeerConnectButton.tsxsrc/modules/remote-access/useNetBirdClient.tssrc/modules/remote-access/vnc/VNCButton.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsxsrc/modules/remote-access/vnc/VNCTooltip.tsxsrc/modules/remote-access/vnc/novnc.d.tssrc/modules/remote-access/vnc/useVNC.tssrc/modules/remote-access/vnc/useVNCQueryParams.tssrc/modules/remote-access/vnc/websocket-proxy.ts
lixmal
force-pushed
the
embedded-vnc
branch
2 times, most recently
from
June 10, 2026 10:24
333e16b to
b44e0d4
Compare
lixmal
force-pushed
the
embedded-vnc
branch
2 times, most recently
from
July 13, 2026 09:13
17102fd to
f5de9d4
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(remote-access)/peer/ssh/page.tsx (1)
181-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLoading message shows IPv4 address even when IPv6 is selected.
The
LoadingMessagehardcodespeer.ip, so when a user selects IPv6 the connecting text still shows the IPv4 address. UsesshHostfor consistency.✏️ Proposed fix
- <LoadingMessage message={`Connecting to ${username}@${peer.ip}...`} /> + <LoadingMessage message={`Connecting to ${username}@${sshHost}...`} />🤖 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 `@src/app/`(remote-access)/peer/ssh/page.tsx at line 181, Update the LoadingMessage in the SSH connection flow to display sshHost instead of peer.ip, ensuring the connecting text reflects the selected IPv4 or IPv6 address.
🤖 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.
Outside diff comments:
In `@src/app/`(remote-access)/peer/ssh/page.tsx:
- Line 181: Update the LoadingMessage in the SSH connection flow to display
sshHost instead of peer.ip, ensuring the connecting text reflects the selected
IPv4 or IPv6 address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 674a8cab-6b28-4046-8b61-4e1437bafbb9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/app/(dashboard)/peer/page.tsxsrc/app/(remote-access)/peer/rdp/page.tsxsrc/app/(remote-access)/peer/ssh/page.tsxsrc/app/(remote-access)/peer/vnc/page.tsxsrc/app/globals.csssrc/auth/SecureProvider.tsxsrc/components/ui/FullScreenLoading.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/app/(dashboard)/peer/page.tsx
- src/app/globals.css
- src/components/ui/FullScreenLoading.tsx
- src/app/(remote-access)/peer/vnc/page.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/modules/remote-access/vnc/VNCToolbar.tsx`:
- Around line 127-141: Update the toolbar’s hover-expanded container around the
End session button to also expand with group-focus-within:max-h-20, and add
visible focus-visible styling to the button so keyboard users can discover and
activate it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c20aa6f0-98af-433c-bc50-89d590cf5b1b
📒 Files selected for processing (2)
src/app/(remote-access)/peer/vnc/page.tsxsrc/modules/remote-access/vnc/VNCToolbar.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/modules/remote-access/vnc/useVNC.ts (3)
205-212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
connectagainst an active session.The early return only covers
VNCStatus.CONNECTING. If a caller invokesconnectwhile the status isCONNECTED, the hook creates a secondRFBinstance and overwritesrfbRef.current, so the previous session and its WebSocket leak. The page currently guards onVNCStatus.DISCONNECTED, so this is defensive only.♻️ Proposed guard
- if (statusRef.current === VNCStatus.CONNECTING) return; + if (statusRef.current !== VNCStatus.DISCONNECTED) return;🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 205 - 212, Update the early-return guard in the connect callback to also reject calls when statusRef.current is VNCStatus.CONNECTED, preventing a second RFB session from replacing the active rfbRef.current instance. Preserve the existing behavior for CONNECTING and all other statuses.
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
dotCursoris accepted but never applied.
connecthardcodesrfb.showDotCursor = falseat Line 262 and ignoresconfig.dotCursor. The VNC page forwardssettings.dotCursorfrom the query parameters, so the user setting silently has no effect. Remove the field fromVNCConfigor honor it inconnect.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` at line 134, Update the VNC connection flow in connect to honor the optional VNCConfig.dotCursor value when assigning rfb.showDotCursor, while preserving the existing default behavior when the option is omitted; alternatively remove dotCursor from VNCConfig and all callers if it is not intended to be supported.
378-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using private noVNC scaling logic.
@novnc/novnc@1.7.0providesscaleViewport, but_updateScaleis not part of the public API. Use only public RFB/Display settings, or keep the custom resize handling in a dedicated module if the built-in behavior is insufficient.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 378 - 384, Update handleResize in useVNC to stop calling the private rfbRef.current._updateScale method. Use only the public scaleViewport API and supported RFB/Display settings, or move any necessary custom resize behavior into a dedicated module while preserving the existing debounced resize handling.src/app/(remote-access)/peer/vnc/page.tsx (1)
134-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate error-notification helper.
The same helper exists in
src/app/(remote-access)/peer/rdp/page.tsx(Lines 65-73). Extract one sharedsendErrorNotificationinto a remote-access utility module and import it in both pages.🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx around lines 134 - 145, Extract the duplicated sendErrorNotification helper from the VNC and RDP page components into a shared remote-access utility module, preserving its notification options and callback behavior. Import and use the shared helper in both pages, removing their local definitions.
🤖 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 `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 502-515: Reset the remote-cursor state in the VNC disconnect
handler alongside setViewOnly(false). Add setShowRemoteCursorState(false) so a
new connection starts with the toolbar and server both showing the remote cursor
as disabled.
- Around line 489-500: The keydown and native paste handlers can send the same
clipboard text twice. In src/modules/remote-access/vnc/useVNC.ts lines 489-500,
update handlePasteShortcut to set a short-lived guard when it performs the eager
paste (and exclude Shift if Ctrl+Shift+V must reach the remote); in
src/app/(remote-access)/peer/vnc/page.tsx lines 488-498, check that guard and
skip vnc.pasteFromClipboardEvent when the keydown path already handled the
gesture.
- Around line 436-443: Update the paste flow around sendRawToVNCProxy and
clipboardPasteFrom so its return value reflects whether keystrokes were actually
sent: preserve the successful true result when the raw proxy send succeeds, but
return the appropriate failure or differentiated outcome after the
clipboard-only fallback. Ensure callers such as VNCToolbar can distinguish a
clipboard update from a successful typed paste.
- Around line 338-359: Update sendClipboard in the VNC setup to stop reading and
transmitting navigator.clipboard contents on every window focus. Trigger
clipboard synchronization only through an explicit user paste action or an
existing session setting, and replace the empty rejection handler with the
established error handling so rejected reads remain diagnosable.
---
Nitpick comments:
In `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 134-145: Extract the duplicated sendErrorNotification helper from
the VNC and RDP page components into a shared remote-access utility module,
preserving its notification options and callback behavior. Import and use the
shared helper in both pages, removing their local definitions.
In `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 205-212: Update the early-return guard in the connect callback to
also reject calls when statusRef.current is VNCStatus.CONNECTED, preventing a
second RFB session from replacing the active rfbRef.current instance. Preserve
the existing behavior for CONNECTING and all other statuses.
- Line 134: Update the VNC connection flow in connect to honor the optional
VNCConfig.dotCursor value when assigning rfb.showDotCursor, while preserving the
existing default behavior when the option is omitted; alternatively remove
dotCursor from VNCConfig and all callers if it is not intended to be supported.
- Around line 378-384: Update handleResize in useVNC to stop calling the private
rfbRef.current._updateScale method. Use only the public scaleViewport API and
supported RFB/Display settings, or move any necessary custom resize behavior
into a dedicated module while preserving the existing debounced resize handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71269112-faec-4eaf-8f7c-f510d03892e6
📒 Files selected for processing (2)
src/app/(remote-access)/peer/vnc/page.tsxsrc/modules/remote-access/vnc/useVNC.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/modules/remote-access/vnc/useVNC.ts (3)
205-212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
connectagainst an active session.The early return only covers
VNCStatus.CONNECTING. If a caller invokesconnectwhile the status isCONNECTED, the hook creates a secondRFBinstance and overwritesrfbRef.current, so the previous session and its WebSocket leak. The page currently guards onVNCStatus.DISCONNECTED, so this is defensive only.♻️ Proposed guard
- if (statusRef.current === VNCStatus.CONNECTING) return; + if (statusRef.current !== VNCStatus.DISCONNECTED) return;🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 205 - 212, Update the early-return guard in the connect callback to also reject calls when statusRef.current is VNCStatus.CONNECTED, preventing a second RFB session from replacing the active rfbRef.current instance. Preserve the existing behavior for CONNECTING and all other statuses.
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
dotCursoris accepted but never applied.
connecthardcodesrfb.showDotCursor = falseat Line 262 and ignoresconfig.dotCursor. The VNC page forwardssettings.dotCursorfrom the query parameters, so the user setting silently has no effect. Remove the field fromVNCConfigor honor it inconnect.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` at line 134, Update the VNC connection flow in connect to honor the optional VNCConfig.dotCursor value when assigning rfb.showDotCursor, while preserving the existing default behavior when the option is omitted; alternatively remove dotCursor from VNCConfig and all callers if it is not intended to be supported.
378-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using private noVNC scaling logic.
@novnc/novnc@1.7.0providesscaleViewport, but_updateScaleis not part of the public API. Use only public RFB/Display settings, or keep the custom resize handling in a dedicated module if the built-in behavior is insufficient.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 378 - 384, Update handleResize in useVNC to stop calling the private rfbRef.current._updateScale method. Use only the public scaleViewport API and supported RFB/Display settings, or move any necessary custom resize behavior into a dedicated module while preserving the existing debounced resize handling.src/app/(remote-access)/peer/vnc/page.tsx (1)
134-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate error-notification helper.
The same helper exists in
src/app/(remote-access)/peer/rdp/page.tsx(Lines 65-73). Extract one sharedsendErrorNotificationinto a remote-access utility module and import it in both pages.🤖 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 `@src/app/`(remote-access)/peer/vnc/page.tsx around lines 134 - 145, Extract the duplicated sendErrorNotification helper from the VNC and RDP page components into a shared remote-access utility module, preserving its notification options and callback behavior. Import and use the shared helper in both pages, removing their local definitions.
🤖 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 `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 502-515: Reset the remote-cursor state in the VNC disconnect
handler alongside setViewOnly(false). Add setShowRemoteCursorState(false) so a
new connection starts with the toolbar and server both showing the remote cursor
as disabled.
- Around line 489-500: The keydown and native paste handlers can send the same
clipboard text twice. In src/modules/remote-access/vnc/useVNC.ts lines 489-500,
update handlePasteShortcut to set a short-lived guard when it performs the eager
paste (and exclude Shift if Ctrl+Shift+V must reach the remote); in
src/app/(remote-access)/peer/vnc/page.tsx lines 488-498, check that guard and
skip vnc.pasteFromClipboardEvent when the keydown path already handled the
gesture.
- Around line 436-443: Update the paste flow around sendRawToVNCProxy and
clipboardPasteFrom so its return value reflects whether keystrokes were actually
sent: preserve the successful true result when the raw proxy send succeeds, but
return the appropriate failure or differentiated outcome after the
clipboard-only fallback. Ensure callers such as VNCToolbar can distinguish a
clipboard update from a successful typed paste.
- Around line 338-359: Update sendClipboard in the VNC setup to stop reading and
transmitting navigator.clipboard contents on every window focus. Trigger
clipboard synchronization only through an explicit user paste action or an
existing session setting, and replace the empty rejection handler with the
established error handling so rejected reads remain diagnosable.
---
Nitpick comments:
In `@src/app/`(remote-access)/peer/vnc/page.tsx:
- Around line 134-145: Extract the duplicated sendErrorNotification helper from
the VNC and RDP page components into a shared remote-access utility module,
preserving its notification options and callback behavior. Import and use the
shared helper in both pages, removing their local definitions.
In `@src/modules/remote-access/vnc/useVNC.ts`:
- Around line 205-212: Update the early-return guard in the connect callback to
also reject calls when statusRef.current is VNCStatus.CONNECTED, preventing a
second RFB session from replacing the active rfbRef.current instance. Preserve
the existing behavior for CONNECTING and all other statuses.
- Line 134: Update the VNC connection flow in connect to honor the optional
VNCConfig.dotCursor value when assigning rfb.showDotCursor, while preserving the
existing default behavior when the option is omitted; alternatively remove
dotCursor from VNCConfig and all callers if it is not intended to be supported.
- Around line 378-384: Update handleResize in useVNC to stop calling the private
rfbRef.current._updateScale method. Use only the public scaleViewport API and
supported RFB/Display settings, or move any necessary custom resize behavior
into a dedicated module while preserving the existing debounced resize handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71269112-faec-4eaf-8f7c-f510d03892e6
📒 Files selected for processing (2)
src/app/(remote-access)/peer/vnc/page.tsxsrc/modules/remote-access/vnc/useVNC.ts
🛑 Comments failed to post (4)
src/modules/remote-access/vnc/useVNC.ts (4)
338-359: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Firefox navigator.clipboard.readText user activation paste prompt behavior💡 Result:
In Firefox, the navigator.clipboard.readText method requires transient user activation to function, ensuring that clipboard access is initiated by the user [1][2][3]. When a script calls navigator.clipboard.readText, Firefox enforces a security model to protect user privacy: 1. Requirement for User Activation: The call must occur within the context of a transient user gesture (such as a click or keypress) [4][2][3]. If the call is made without this recent user interaction, the promise will be rejected with a DOMException (e.g., "Clipboard read request was blocked due to lack of user activation") [4][3]. 2. Paste Prompt Behavior: If the data on the clipboard did not originate from the same-origin page, Firefox will display a "Paste" context menu or prompt to the user [2][5]. The user must explicitly interact with this prompt to grant the page access to the clipboard contents [2][5]. If the user dismisses this prompt, the promise will be rejected [4]. 3. Same-Origin Exception: If the clipboard content originated from the same-origin page, access may be granted without an additional prompt, provided the transient user activation requirement is still met [2][5]. 4. Browser Context: This behavior is enabled by default in modern Firefox versions (starting from version 125/127 for different parts of the API) [6][7][2]. Browser extensions with appropriate permissions (e.g., clipboardRead) may be exempt from these specific user-activation and prompt requirements depending on their configuration [8][5]. All interactions with the Clipboard API must also occur within a secure context (HTTPS) [9][10].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API
- 2: https://groups.google.com/a/mozilla.org/g/dev-platform/c/lNXj_A-Lllk
- 3: https://raw.githubusercontent.com/mozilla-firefox/firefox/main/dom/events/Clipboard.cpp
- 4: https://bugzilla.mozilla.org/show_bug.cgi?id=1778437
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1809106
- 6: https://caniuse.com/mdn-api_clipboard_readtext
- 7: https://bugzilla.mozilla.org/show_bug.cgi?id=1877400
- 8: https://bugzilla.mozilla.org/show_bug.cgi?id=1773681
- 9: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/readText
- 10: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the relevant file and surrounding code without executing repository code. if [ -f src/modules/remote-access/vnc/useVNC.ts ]; then echo "== file exists ==" wc -l src/modules/remote-access/vnc/useVNC.ts echo echo "== relevant clipboard section 320-370 ==" sed -n '320,370p' src/modules/remote-access/vnc/useVNC.ts | nl -ba -v320 echo echo "== relevant Firefox/clipboard comment section 430-460 ==" sed -n '430,460p' src/modules/remote-access/vnc/useVNC.ts | nl -ba -v430 else echo "file not found" fi echo echo "== occurrences of sendClipboard and clipboardPasteFrom/readText in tracked files ==" rg -n "sendClipboard|clipboardPasteFrom|clipboard\.readText|Paste toolbar|window\.addEventListener\(\"focus\"" .Repository: netbirdio/dashboard
Length of output: 302
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== relevant clipboard section 320-370 ==" sed -n '320,370p' src/modules/remote-access/vnc/useVNC.ts | awk '{printf "%5d: %s\n", 319+NR, $0}' echo echo "== relevant Firefox/clipboard comment section 430-460 ==" sed -n '430,460p' src/modules/remote-access/vnc/useVNC.ts | awk '{printf "%5d: %s\n", 429+NR, $0}' echo echo "== occurrences of sendClipboard and clipboardPasteFrom/readText in tracked files ==" rg -n "sendClipboard|clipboardPasteFrom|readText|Paste toolbar|window\.addEventListener\(\"focus\"" .Repository: netbirdio/dashboard
Length of output: 5416
Avoid reading the host clipboard without an explicit paste action.
sendClipboard()readsnavigator.clipboard.readText()on every window focus and sends the result to the remote peer. A focus-based read can also be rejected by Firefox when the clipboard did not originate from the same-origin page. Keep this sync behind an explicit user action or session setting, and handle rejected clipboard reads without suppressing useful paste failures.🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 338 - 359, Update sendClipboard in the VNC setup to stop reading and transmitting navigator.clipboard contents on every window focus. Trigger clipboard synchronization only through an explicit user paste action or an existing session setting, and replace the empty rejection handler with the established error handling so rejected reads remain diagnosable.
436-443: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return value does not reflect the fallback outcome.
When
sendRawToVNCProxyreturnsfalse, the function only writes to the remote clipboard throughclipboardPasteFrom. No keystrokes are typed. The function still returnstrue, so callers such asVNCToolbarreport a successful paste. Return the actual result, or return a discriminated outcome so the UI can phrase the message correctly.🐛 Proposed fix
const sent = sendRawToVNCProxy(proxyIDRef.current, buf); if (!sent) { // Fall back to standard CutText so we at least update the OS clipboard // when the type path isn't available. rfbRef.current.clipboardPasteFrom(text); } rfbRef.current.focus(); - return true; + return sent;📝 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.const sent = sendRawToVNCProxy(proxyIDRef.current, buf); if (!sent) { // Fall back to standard CutText so we at least update the OS clipboard // when the type path isn't available. rfbRef.current.clipboardPasteFrom(text); } rfbRef.current.focus(); return sent;🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 436 - 443, Update the paste flow around sendRawToVNCProxy and clipboardPasteFrom so its return value reflects whether keystrokes were actually sent: preserve the successful true result when the raw proxy send succeeds, but return the appropriate failure or differentiated outcome after the clipboard-only fallback. Ensure callers such as VNCToolbar can distinguish a clipboard update from a successful typed paste.
489-500: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Two paste paths can deliver the same text twice. The keydown interception and the native paste handler both type text on the remote. The shared root cause is that
handlePasteShortcutpastes eagerly instead of leaving the paste gesture to thepasteevent, and neither path knows about the other.
src/modules/remote-access/vnc/useVNC.ts#L489-L500: add a short-lived guard set byhandlePasteShortcutand checked by the paste-event path, or restrict this handler to browsers that withhold thepasteevent. Also add&& !e.shiftKeyif Ctrl+Shift+V must reach the remote.src/app/(remote-access)/peer/vnc/page.tsx#L488-L498: skipvnc.pasteFromClipboardEventwhen the guard indicates that the keydown path already pasted.📍 Affects 2 files
src/modules/remote-access/vnc/useVNC.ts#L489-L500(this comment)src/app/(remote-access)/peer/vnc/page.tsx#L488-L498🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 489 - 500, The keydown and native paste handlers can send the same clipboard text twice. In src/modules/remote-access/vnc/useVNC.ts lines 489-500, update handlePasteShortcut to set a short-lived guard when it performs the eager paste (and exclude Shift if Ctrl+Shift+V must reach the remote); in src/app/(remote-access)/peer/vnc/page.tsx lines 488-498, check that guard and skip vnc.pasteFromClipboardEvent when the keydown path already handled the gesture.
502-515: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset
showRemoteCursoron disconnect.The disconnect handler at Line 306 resets
viewOnlybut leavesshowRemoteCursor. After a view-only session ends and the user reconnects, the toolbar still shows the remote cursor as enabled while the server starts with it disabled. AddsetShowRemoteCursorState(false)next tosetViewOnly(false).🤖 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 `@src/modules/remote-access/vnc/useVNC.ts` around lines 502 - 515, Reset the remote-cursor state in the VNC disconnect handler alongside setViewOnly(false). Add setShowRemoteCursorState(false) so a new connection starts with the toolbar and server both showing the remote cursor as disabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a browser VNC viewer for peers, mirroring the existing SSH/RDP integration. Traffic is tunneled through the NetBird WASM client, so no extra ports are exposed to the browser.
/peer/vncpage wired into the peer overview and dropdown alongside SSH/RDP@novnc/novncnpm package and route its WebSocket through avnc.proxy.localinterceptor that bridges to the Go WASM proxyBefore merge
src/utils/config.ts(wasmPathdefault, currentlyhttps://pkgs.netbird.io/wasm/client/v0.74.2) to a build that carries the embedded VNC server. Has to wait until the netbird client PR is merged and a matching WASM client is published, otherwise the viewer talks to a client that cannot serve VNC.Issue ticket number and link
Documentation
Select exactly one:
Docs will follow once the feature is enabled by default.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit