Add embedded VNC viewer#643
Open
lixmal wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughChangesVNC Remote Access Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VNCButton
participant VNCPage
participant NetBird as NetBird client
participant useVNC
participant noVNC as noVNC RFB
User->>VNCButton: Open VNC
VNCButton->>VNCPage: Navigate with peer query
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📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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
333e16b to
b44e0d4
Compare
lixmal
force-pushed
the
embedded-vnc
branch
2 times, most recently
from
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
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 proxyIssue 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