-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: add restart sandbox button to toolbar #2843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
34bfd35
804a1ea
a3f45d2
fa834eb
0025f42
da4ab4d
f897610
4cb140a
ab76b25
45456c8
5d7cc0e
ccf112f
cb0293e
eaaa718
ebc7297
dcfada1
367678f
1638260
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,14 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'use client'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useEditorEngine } from '@/components/store/editor'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Icons } from '@onlook/ui/icons'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { toast } from '@onlook/ui/sonner'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Tabs, TabsContent, TabsList, TabsTrigger } from '@onlook/ui/tabs'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Tooltip, TooltipContent, TooltipTrigger } from '@onlook/ui/tooltip'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { cn } from '@onlook/ui/utils'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { observer } from 'mobx-react-lite'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { motion } from 'motion/react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useEffect, useRef, useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Terminal } from './terminal'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const TerminalArea = observer(({ children }: { children: React.ReactNode }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -47,12 +50,146 @@ export const TerminalArea = observer(({ children }: { children: React.ReactNode | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [terminalHidden, setTerminalHidden] = useState(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [restarting, setRestarting] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ultra-lightweight error detection using a single timer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [hasSandboxError, setHasSandboxError] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const mountTimeRef = useRef<number>(Date.now()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Single effect that only sets/clears one timer - extremely efficient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Clear any existing timer first | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (timeoutIdRef.current) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(timeoutIdRef.current); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutIdRef.current = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const activeBranch = branches.activeBranch; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!activeBranch) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const branchData = branches.getBranchDataById(activeBranch.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const sandbox = branchData?.sandbox; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Quick bailouts - no timer needed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!sandbox?.session) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If we have a provider, we're connected - no error | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (sandbox.session.provider) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if enough time has passed since mount (avoid false positives on initial load) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const timeSinceMount = Date.now() - mountTimeRef.current; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (timeSinceMount < 5000) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set timer to check after 5 seconds from mount | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const delay = 5000 - timeSinceMount; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutIdRef.current = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if still no provider (indicates 502 or failure) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (stillNoProvider) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, delay); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // We're past the grace period - if no provider, it's an error | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Cleanup on unmount or deps change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (timeoutIdRef.current) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(timeoutIdRef.current); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutIdRef.current = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [branches.activeBranch?.id, branches]); // Only re-run when branch ID changes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Extract restart logic into a reusable function to follow DRY principles | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleRestartSandbox = async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const activeBranch = branches.activeBranch; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!activeBranch || restarting) return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setRestarting(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(false); // Clear error state on restart | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const sandbox = branches.getSandboxById(activeBranch.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!sandbox?.session) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toast.error('Sandbox session not available'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setRestarting(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const success = await sandbox.session.restartDevServer(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (success) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toast.success('Sandbox restarted successfully', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| icon: <Icons.Cube className="h-4 w-4" />, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Wait 5 seconds before refreshing webviews to avoid 502 errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const frames = editorEngine.frames.getAll(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| frames.forEach(frame => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| editorEngine.frames.reloadView(frame.frame.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (frameError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error('Failed to reload frame:', frame.frame.id, frameError); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setRestarting(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, 5000); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setTimeout(() => { | |
| const frames = editorEngine.frames.getAll(); | |
| frames.forEach(frame => { | |
| try { | |
| editorEngine.frames.reloadView(frame.frame.id); | |
| } catch (frameError) { | |
| console.error('Failed to reload frame:', frame.frame.id, frameError); | |
| } | |
| }); | |
| setRestarting(false); | |
| }, 5000); | |
| const timerRef = useRef<NodeJS.Timeout>(); | |
| useEffect(() => { | |
| timerRef.current = setTimeout(() => { | |
| const frames = editorEngine.frames.getAll(); | |
| frames.forEach(frame => { | |
| try { | |
| editorEngine.frames.reloadView(frame.frame.id); | |
| } catch (frameError) { | |
| console.error('Failed to reload frame:', frame.frame.id, frameError); | |
| } | |
| }); | |
| setRestarting(false); | |
| }, 5000); | |
| return () => { | |
| if (timerRef.current) { | |
| clearTimeout(timerRef.current); | |
| } | |
| }; | |
| }, [editorEngine]); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix memory leak and improve error handling.
The setTimeout creates a timer that is never cleaned up if the component unmounts during the 5-second delay. The restarting state management also needs improvement to prevent double submits.
Apply this diff to fix the memory leak and improve state management:
const handleRestartSandbox = async () => {
const activeBranch = branches.activeBranch;
if (!activeBranch || restarting) return;
setRestarting(true);
setHasSandboxError(false); // Clear error state on restart
+
+ let timeoutId: NodeJS.Timeout | null = null;
try {
const sandbox = branches.getSandboxById(activeBranch.id);
if (!sandbox?.session) {
toast.error('Sandbox session not available');
- setRestarting(false);
return;
}
const success = await sandbox.session.restartDevServer();
if (success) {
toast.success('Sandbox restarted successfully', {
icon: <Icons.Cube className="h-4 w-4" />,
});
// Wait 5 seconds before refreshing webviews to avoid 502 errors
- setTimeout(() => {
- const frames = editorEngine.frames.getAll();
- frames.forEach(frame => {
- try {
- editorEngine.frames.reloadView(frame.frame.id);
- } catch (frameError) {
- console.error('Failed to reload frame:', frame.frame.id, frameError);
- }
- });
- setRestarting(false);
- }, 5000);
+ await new Promise<void>((resolve) => {
+ timeoutId = setTimeout(() => {
+ const frames = editorEngine.frames.getAll();
+ frames.forEach(frame => {
+ try {
+ editorEngine.frames.reloadView(frame.frame.id);
+ } catch (frameError) {
+ console.error('Failed to reload frame:', frame.frame.id, frameError);
+ }
+ });
+ resolve();
+ }, 5000);
+ });
} else {
toast.error('Failed to restart sandbox');
- setRestarting(false);
}
} catch (error) {
console.error('Error restarting sandbox:', error);
toast.error('An error occurred while restarting the sandbox');
- setRestarting(false);
+ } finally {
+ setRestarting(false);
}
+
+ // Cleanup function to clear timeout if component unmounts
+ return () => {
+ if (timeoutId) {
+ clearTimeout(timeoutId);
+ }
+ };
};Store the cleanup function in a ref to handle component unmounting:
const timeoutIdRef = useRef<NodeJS.Timeout | null>(null);
+const restartCleanupRef = useRef<(() => void) | null>(null);
+useEffect(() => {
+ return () => {
+ // Cleanup any pending restart timeouts on unmount
+ if (restartCleanupRef.current) {
+ restartCleanupRef.current();
+ }
+ };
+}, []);Then in the handler:
- return () => {
- if (timeoutId) {
- clearTimeout(timeoutId);
- }
- };
+ restartCleanupRef.current = () => {
+ if (timeoutId) {
+ clearTimeout(timeoutId);
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
around lines 116-158, the setTimeout used to delay frame reloads can leak if the
component unmounts and the restarting state handling allows duplicate submits;
store the timeout ID in a ref (e.g., restartTimeoutRef) and check restarting at
the top of the handler to prevent double submits, clear any existing timeout
before setting a new one, ensure setRestarting(false) is called in every exit
path (success, failure, and catch), and add a useEffect cleanup that clears the
stored timeout on unmount to avoid the memory leak.
Uh oh!
There was an error while loading. Please reload this page.