Skip to content

Conversation

zstarer
Copy link

@zstarer zstarer commented Aug 17, 2025

adds a fullscreen option for vnc.

Screenshot 2025-08-17 at 3 00 59 PM

@atupem atupem changed the base branch from main to development August 19, 2025 13:48
@atupem atupem self-requested a review September 3, 2025 18:14
Copy link
Contributor

@atupem atupem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aspect ratio of the desktop is changed once you exit fullscreen

@executiveusa
Copy link

Thanks for adding the fullscreen feature! I've reviewed the code and found a few issues that need to be addressed:

  1. Add fullscreen exit handler to maintain aspect ratio:
    ` ypescript
    // Add event listener for fullscreen changes
    useEffect(() => {
    const handleFullscreenChange = () => {
    if (!document.fullscreenElement && vncContainerRef.current) {
    // Force a resize calculation when exiting fullscreen
    updateSize();
    }
    };

document.addEventListener('fullscreenchange', handleFullscreenChange);
return () => document.removeEventListener('fullscreenchange', handleFullscreenChange);
}, []);
`

  1. Fix the aspect ratio comments and logic:
    ypescript if (parentWidth / parentHeight > aspectRatio) { // Parent is wider than needed - height is the limiting factor height = parentHeight; width = height * aspectRatio; } else { // Parent is taller than needed - width is the limiting factor width = parentWidth; height = width / aspectRatio; }

  2. Consider making the aspect ratio configurable:
    ` ypescript
    interface DesktopContainerProps {
    // ... existing props
    aspectRatio?: number; // Optional prop with default 4:3
    }

// In the component:
const aspectRatio = props.aspectRatio ?? (1280 / 960);
`

These changes should resolve the aspect ratio issues when exiting fullscreen mode.

Copy link

@executiveusa executiveusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to suggest a CSS transform-based solution for the desktop container's fullscreen functionality. Here's the implementation:

export const DesktopContainer: React.FC<DesktopContainerProps> = ({
  children,
  className = "",
  status = "running",
}) => {
  const vncContainerRef = useRef<HTMLDivElement>(null);
  const [isFullscreen, setIsFullscreen] = useState(false);

  useEffect(() => {
    const handleFullscreenChange = () => {
      setIsFullscreen(!!document.fullscreenElement);
    };

    document.addEventListener("fullscreenchange", handleFullscreenChange);
    return () => {
      document.removeEventListener("fullscreenchange", handleFullscreenChange);
    };
  }, []);

  const handleFullScreen = async () => {
    if (!vncContainerRef.current) return;

    try {
      if (!document.fullscreenElement) {
        await vncContainerRef.current.requestFullscreen();
      } else {
        await document.exitFullscreen();
      }
    } catch (err) {
      console.error("Fullscreen error:", err);
    }
  };

  return (
    <div className={cn(
      "border-bytebot-bronze-light-7 flex w-full flex-col rounded-t-lg border-t border-r border-l",
      className
    )}>
      {/* Header */}
      <div className="flex items-center justify-between p-2 bg-gray-50">
        {/* Status */}
        <div className="flex items-center gap-2">
          <div className={cn("h-2 w-2 rounded-full", {
            "bg-green-500": status === "running",
            "bg-yellow-500": status === "starting",
            "bg-red-500": status === "error" || status === "stopped",
          })} />
          <span className="text-sm capitalize">{status}</span>
        </div>

        {/* Actions */}
        <div className="flex items-center gap-2">
          {children}
          <button
            onClick={handleFullScreen}
            className={cn(
              "px-4 py-2 rounded font-medium transition-colors",
              "bg-gray-200 hover:bg-gray-300 text-gray-800",
              "focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-gray-500"
            )}
          >
            {isFullscreen ? "Exit Fullscreen" : "Fullscreen"}
          </button>
        </div>
      </div>

      {/* VNC Container */}
      <div
        ref={vncContainerRef}
        className={cn(
          "relative overflow-hidden bg-gray-900",
          {
            "h-[75vh]": !isFullscreen,
            "fixed inset-0 z-50": isFullscreen
          }
        )}
      >
        {/* VNC Screen */}
        <div className={cn(
          "absolute left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2",
          "w-full h-full max-w-full max-h-full",
          "transition-all duration-200 ease-in-out",
          {
            "aspect-[4/3] w-auto": !isFullscreen,
            "w-screen h-screen": isFullscreen
          }
        )}>
          {status === "running" && children}
        </div>
      </div>
    </div>
  );
};

Key improvements:

  1. Uses CSS transforms for reliable centering
  2. Maintains aspect ratio using CSS properties
  3. Adds smooth transitions between states
  4. Improves fullscreen handling with fixed positioning
  5. Cleans up component structure

This solution provides better reliability and maintainability while preserving the desired functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants