-
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 13 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,147 @@ 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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only set timer if actually connecting (not just idle) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const isConnecting = sandbox.session.isConnecting || sandbox.isIndexing; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isConnecting) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setHasSandboxError(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set a single 5-second timer - that's it, ultra simple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutIdRef.current = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Final check after 5 seconds | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const stillConnecting = branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.isConnecting || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branches.getBranchDataById(activeBranch.id)?.sandbox?.isIndexing; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const stillConnecting = branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.isConnecting || | |
| branches.getBranchDataById(activeBranch.id)?.sandbox?.isIndexing; | |
| const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider; | |
| const branchData = branches.getBranchDataById(activeBranch.id); | |
| const stillConnecting = branchData?.sandbox?.session?.isConnecting || | |
| branchData?.sandbox?.isIndexing; | |
| const stillNoProvider = !branchData?.sandbox?.session?.provider; |
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.
Localize user‑facing text via next‑intl (repo guideline)
Strings are hardcoded; guidelines require using next-intl hooks/messages.
Illustrative diff (apply pattern to all messages/tooltips/labels):
@@
-import { toast } from '@onlook/ui/sonner';
+import { toast } from '@onlook/ui/sonner';
+import { useTranslations } from 'next-intl';
@@
export const TerminalArea = observer(({ children }: { children: React.ReactNode }) => {
+ const t = useTranslations('TerminalArea');
@@
- toast.error('Sandbox session not available');
+ toast.error(t('sandboxSessionUnavailable'));
@@
- if (!opts?.silent) {
- toast.success('Sandbox restarted successfully', {
+ if (!opts?.silent) {
+ toast.success(t('restartSuccess'), {
icon: <Icons.Cube className="h-4 w-4" />,
});
}
@@
- if (!opts?.silent) toast.error('Failed to restart sandbox');
+ if (!opts?.silent) toast.error(t('restartFailed'));
@@
- if (!opts?.silent) toast.error('An error occurred while restarting the sandbox');
+ if (!opts?.silent) toast.error(t('restartErrorGeneric'));
@@
- <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent>
+ <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent>
@@
- <TooltipContent sideOffset={5} hideArrow>Toggle Terminal</TooltipContent>
+ <TooltipContent sideOffset={5} hideArrow>{t('toggleTerminal')}</TooltipContent>
@@
- <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent>
+ <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent>
@@
- <TooltipContent sideOffset={5} hideArrow>Toggle Terminal</TooltipContent>
+ <TooltipContent sideOffset={5} hideArrow>{t('toggleTerminal')}</TooltipContent>
@@
- Terminal
+ {t('terminal')}
@@
- <span className="text-sm">No terminal sessions available</span>
+ <span className="text-sm">{t('noTerminalSessions')}</span>Define the above keys in your messages file under the TerminalArea namespace.
Also applies to: 107-110, 125-126, 130-131, 164-165, 175-176, 218-219, 229-230, 274-275, 190-191, 3-5, 14-17
🤖 Prompt for AI Agents
In apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
around lines 100-101 (and also apply same change to lines 107-110, 125-126,
130-131, 164-165, 175-176, 190-191, 218-219, 229-230, 274-275, and header lines
3-5, 14-17), replace all hardcoded user-facing strings (toasts, button labels,
tooltips, etc.) with next-intl message lookups: import and use the next-intl
hook (e.g., const t = useTranslations('TerminalArea')) and call t('keyName') for
each string, add corresponding keys under the TerminalArea namespace in the
locale messages file, and ensure any dynamic values use t('key', { value }) or
template usage supported by next-intl; update tests/types if needed to expect
translated keys.
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.
Resource leak bug: The setTimeout creates a timer that is never cleaned up if the component unmounts before the 5-second delay completes. This can cause memory leaks and attempts to update unmounted components. The timer should be stored in a ref and cleared in a useEffect cleanup function.
| 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.