feat: add restart sandbox button to toolbar #2843
Conversation
Add a rotate icon button next to the terminal icon that allows users to restart the sandbox without using chat credits. The button is disabled when no branch is selected. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a RestartSandboxButton component and renders it in TerminalArea headers, introduces a duplicated RestartSandbox icon in the icons map, adds FramesManager.getByBranchId, and replaces CLAUDE.md with a comprehensive Onlook Agents Guide. Restart flow includes readiness detection, restartDevServer call, toasts, and a 5s post-restart frame reload. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as TerminalArea
participant Button as RestartSandboxButton
participant Branches as editorStore.branches
participant SandboxStore as getSandboxById
participant Session as sandbox.session
participant Toast as toast
participant Delay5s as 5s delay
participant Frames as editorEngine.frames
note right of UI #FFF4CC: hasSandboxError derived\nfrom per-branch 5s readiness timeout
User->>UI: Click restart (or button shown)
UI->>Button: handleRestartSandbox()
Button->>Branches: read activeBranch
alt no activeBranch or already restarting
Button-->>User: no-op (disabled)
else activeBranch present
Button->>SandboxStore: getSandboxById(activeBranch.id)
SandboxStore-->>Button: sandbox | undefined
alt sandbox && sandbox.session
Button->>Session: restartDevServer()
Session-->>Button: success / failure / error
alt success
Button->>Toast: show loading -> success
Button->>Delay5s: wait 5s
Delay5s-->>Button: continue
Button->>Frames: reloadView(all frames for branch)
Frames-->>Button: results
else failure/error
Button->>Toast: show error
end
else
Button->>Toast: show error "Sandbox session not available"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
59-67: Confirm restartDevServer behavior and add caller error handlingVerified: async restartDevServer(): Promise exists at apps/web/client/src/components/store/editor/sandbox/session.ts and is called from apps/web/client/src/components/tools/handlers/sandbox.ts and apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (around lines 59–67 and 112–120). The terminal-area onClick awaits the call but does not check the boolean result or catch exceptions.
Actionable items:
- Ensure restartDevServer consistently handles internal errors (catch/log and return false or throw a documented error).
- Update terminal-area onClick to handle the returned boolean and wrap the await in try/catch so failures surface to the user (show success/error feedback, matching handlers/sandbox.ts behavior).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
Outdated
Show resolved
Hide resolved
- Changed icon from Rotate to Reload to match webframe styling - Added toast notification with cube icon on successful restart - Show error toast if restart fails 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| onClick={async () => { | ||
| const activeBranch = branches.activeBranch; | ||
| if (activeBranch) { | ||
| const sandbox = branches.getSandboxById(activeBranch.id); | ||
| if (sandbox?.session) { | ||
| await sandbox.session.restartDevServer(); | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
The expanded view's restart button is missing the toast notifications that are present in the collapsed view implementation. To maintain consistent user feedback, please add the same success and error toast notifications:
const success = await sandbox.session.restartDevServer();
if (success) {
toast.success('Sandbox restarted successfully', {
icon: <Icons.Cube className="h-4 w-4" />,
});
} else {
toast.error('Failed to restart sandbox');
}This ensures users receive appropriate feedback regardless of which button they use to restart the sandbox.
| onClick={async () => { | |
| const activeBranch = branches.activeBranch; | |
| if (activeBranch) { | |
| const sandbox = branches.getSandboxById(activeBranch.id); | |
| if (sandbox?.session) { | |
| await sandbox.session.restartDevServer(); | |
| } | |
| } | |
| }} | |
| onClick={async () => { | |
| const activeBranch = branches.activeBranch; | |
| if (activeBranch) { | |
| const sandbox = branches.getSandboxById(activeBranch.id); | |
| if (sandbox?.session) { | |
| const success = await sandbox.session.restartDevServer(); | |
| if (success) { | |
| toast.success('Sandbox restarted successfully', { | |
| icon: <Icons.Cube className="h-4 w-4" />, | |
| }); | |
| } else { | |
| toast.error('Failed to restart sandbox'); | |
| } | |
| } | |
| } | |
| }} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
- Extract restart logic into reusable handleRestartSandbox function (DRY principle) - Add comprehensive error handling with try-catch blocks - Add error logging for debugging - Ensure consistent toast notifications in both views - Add frame-specific error handling when refreshing webviews - Add custom RestartSandbox icon with cube and refresh symbol 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
93-109: Prior feedback addressed (DRY + toasts).Restart flow extracted and both buttons share it with error handling and toasts. Nice.
Also applies to: 138-154
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (5)
60-61: Externalize user‑facing strings to next‑intl.Avoid hardcoded UI text per repo guidelines.
+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')); ... - toast.success('Sandbox restarted successfully', { icon: <Icons.Cube className="h-4 w-4" /> }); + toast.success(t('restartSuccess'), { icon: <Icons.Cube className="h-4 w-4" /> }); ... - toast.error('Failed to restart sandbox'); + toast.error(t('restartFailure')); ... - toast.error('An error occurred while restarting the sandbox'); + toast.error(t('restartError')); ... - <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent> + <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent> ... - <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent> + <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent> ... - <span className="text-sm">No terminal sessions available</span> + <span className="text-sm">{t('noTerminalSessions')}</span>Also applies to: 76-85, 108-109, 153-154, 209-210
95-106: Add aria-labels to icon buttons.Tooltips don’t help screen readers; add labels for a11y.
-<button +<button + aria-label="Restart Sandbox" onClick={handleRestartSandbox} disabled={!branches.activeBranch} className={cn(/* ... */)} >Do the same for the “Toggle Terminal” buttons with aria-label="Toggle Terminal".
Also applies to: 140-151
176-176: Remove defaultValue on a controlled Tabs.Tabs is controlled via value; defaultValue is ignored.
-<Tabs defaultValue={'cli'} value={activeSessionId || ''} onValueChange={(value) => { +<Tabs value={activeSessionId || ''} onValueChange={(value) => {
17-18: Avoidanyin session typing.Per guidelines, don’t use
any. You don’t usesessiondownstream—drop it or type asunknown.-const allTerminalSessions = new Map<string, { name: string; branchName: string; branchId: string; sessionId: string; session: any }>(); +const allTerminalSessions = new Map<string, { name: string; branchName: string; branchId: string; sessionId: string }>();And remove the
sessionfield where set.
39-41: Guard activeBranch access.Avoid property access on possibly undefined activeBranch.
- if (branch.id === branches.activeBranch.id && sessionId === sandbox.session.activeTerminalSessionId) { + if (branches.activeBranch && branch.id === branches.activeBranch.id && sessionId === sandbox.session.activeTerminalSessionId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(3 hunks)packages/ui/src/components/icons/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsxpackages/ui/src/components/icons/index.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
packages/ui/src/components/sonner.tsx (1)
toast(19-19)
🔇 Additional comments (2)
packages/ui/src/components/icons/index.tsx (1)
1671-1696: LGTM for new RestartSandbox icon.Naming and props match existing Icons pattern.
PR description mentions Icons.Rotate; code uses Icons.RestartSandbox. Please update the PR text for accuracy.
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
66-74: ```shell
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
base="apps/web/client/src/components/store"
echo "Checking expected editor file locations under $base..."
for candidate in "$base"/editor.{ts,tsx,js,jsx} "$base"/index.{ts,tsx,js,jsx}; do
if [ -f "$candidate" ]; then
echo "---- FOUND: $candidate ----"
sed -n '1,240p' "$candidate" || true
fi
doneecho
echo "Fallback: search for files that mention useEditorEngine or editorEngine"
grep -RIn --exclude-dir={.git,node_modules,dist,build} "useEditorEngine" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "editorEngine" || trueecho
echo "Search for frames API usages (getAll, reloadView, reloadBranchViews) and branchId/frame.id shapes"
grep -RIn --exclude-dir={.git,node_modules,dist,build} "frames.getAll" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "reloadView" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "reloadBranchViews" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "branchId" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "frame.frame.id" || true
grep -RIn --exclude-dir={.git,node_modules,dist,build} "frame.id" || trueecho
echo "Search for Frames type/class/interface"
grep -RIn --exclude-dir={.git,node_modules,dist,build} -E "class .*Frames|interface .*Frames|type .*Frames" || trueecho
echo "Search for 'frames' property assignments in editor engine"
grep -RIn --exclude-dir={.git,node_modules,dist,build} -E "frames\s*[:=]" || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
Outdated
Show resolved
Hide resolved
- Add 'use client' directive as required for client components - Add 5-second delay before refreshing webviews to avoid 502 errors - Show loading spinner during restart operation - Prevent double submits with restarting flag - Update RestartSandbox icon to larger, more legible version - Improve error handling to properly clear loading state 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/components/icons/index.tsx (1)
133-136: Avoidanyin IconProps; use SVG props instead.
[key: string]: anyweakens type safety and violates TS guidelines. Prefer the built‑in React SVG props.Apply:
-export interface IconProps { - className?: string; - [key: string]: any; -} +export type IconProps = React.SVGProps<SVGSVGElement>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(3 hunks)packages/ui/src/components/icons/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
packages/ui/src/components/icons/index.tsx
🔇 Additional comments (1)
packages/ui/src/components/icons/index.tsx (1)
1671-1696: RestartSandbox icon: LGTM (verified)
Single definition at packages/ui/src/components/icons/index.tsx:1671; usages at apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx:119 and 168.
- Add detection for sandbox connection issues (502 errors, timeouts) - Change button to amber colors when sandbox has errors - Background: bg-amber-900, hover:bg-amber-800 - Icon: text-amber-200, hover:text-amber-100 - Check for connection issues every 2 seconds - Clear error state when restart is initiated - Visual indicator helps users know when sandbox needs restarting 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (2)
41-44: Fix potential crash when no active branchAccessing
branches.activeBranch.idcan throw when no branch is selected. Capture the id defensively and guard the comparison.Apply this diff:
@@ - let activeSessionId: string | null = null; + let activeSessionId: string | null = null; + const activeBranchId = branches.activeBranch?.id ?? null; @@ - if (branch.id === branches.activeBranch.id && sessionId === sandbox.session.activeTerminalSessionId) { + if (activeBranchId && branch.id === activeBranchId && sessionId === sandbox.session.activeTerminalSessionId) { activeSessionId = key; }Also applies to: 20-21
19-20: Removeanyand drop unusedsessionfrom the map payloadRepo guideline forbids
any. Also thesessionfield isn’t used later.Apply this diff:
@@ - const allTerminalSessions = new Map<string, { name: string; branchName: string; branchId: string; sessionId: string; session: any }>(); + type TerminalSessionInfo = { + name: string; + branchName: string; + branchId: string; + sessionId: string; + }; + const allTerminalSessions = new Map<string, TerminalSessionInfo>(); @@ - allTerminalSessions.set(key, { - name: session.name, - branchName: branch.name, - branchId: branch.id, - sessionId: sessionId, - session: session - }); + allTerminalSessions.set(key, { + name: session.name, + branchName: branch.name, + branchId: branch.id, + sessionId: sessionId, + });Also applies to: 31-39
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (5)
111-124: Clear the 5s timeout on unmount to avoid setting state after unmountMinor leak/race if the component unmounts within the delay.
Apply this diff:
@@ -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; @@ - const [hasSandboxError, setHasSandboxError] = useState(false); + const [hasSandboxError, setHasSandboxError] = useState(false); + const restartTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); @@ - // Wait 5 seconds before refreshing webviews to avoid 502 errors - setTimeout(() => { + // Wait 5 seconds before refreshing webviews to avoid 502 errors + restartTimeoutRef.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); setHasSandboxError(false); // Clear any error state after successful restart - }, 5000); + }, 5000); @@ - }, [branches.activeBranch, editorEngine.frames, editorEngine.branches]); + }, [branches.activeBranch, editorEngine.frames, editorEngine.branches]); + + // Cleanup any pending restart delay on unmount + useEffect(() => { + return () => { + if (restartTimeoutRef.current) { + clearTimeout(restartTimeoutRef.current); + } + }; + }, []);Also applies to: 52-55, 87-88, 11-11
142-152: Add accessible labels to icon-only restart buttonsIcon-only buttons need
aria-labelfor screen readers.Apply this diff:
-<button +<button + aria-label="Restart Sandbox" onClick={handleRestartSandbox} @@ -<button +<button + aria-label="Restart Sandbox" - onClick={() => handleRestartSandbox({ silent: true })} + onClick={() => handleRestartSandbox({ silent: true })}Also applies to: 196-206
168-171: Add accessible labels to icon-only toggle buttonsSame a11y concern for the terminal toggle.
Apply this diff:
-<button +<button + aria-label="Toggle Terminal" onClick={() => setTerminalHidden(!terminalHidden)}Also applies to: 222-225
241-241: Remove unused defaultValue on controlled Tabs
valuemakes Tabs controlled;defaultValue={'cli'}is ignored and confusing.Apply this diff:
- <Tabs defaultValue={'cli'} value={activeSessionId || ''} onValueChange={(value) => { + <Tabs value={activeSessionId || ''} onValueChange={(value) => {
56-87: Reduce false positives in error polling — add a short threshold (getBranchDataById confirmed)getBranchDataById is defined at apps/web/client/src/components/store/editor/branch/manager.ts; require 3 consecutive polls (~6s) before surfacing sandbox errors in apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx. Apply this corrected diff:
@@ -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; @@ - useEffect(() => { + const connectingStreakRef = useRef(0); + useEffect(() => { const checkForErrors = () => { @@ - const hasConnectionIssue = frames.some(frame => { + const hasConnectionIssue = frames.some(frame => { // Check if frame belongs to active branch if (frame.frame.branchId !== activeBranch.id) return false; // Check for connection timeout or other indicators const branchData = editorEngine.branches.getBranchDataById(frame.frame.branchId); const isConnecting = branchData?.sandbox?.session?.isConnecting || branchData?.sandbox?.isIndexing; - - // If it's been connecting for too long, likely a 502 or connection issue - return isConnecting; + return Boolean(isConnecting); }); - setHasSandboxError(hasConnectionIssue); + // Require 3 consecutive polls (~6s) before surfacing an error + if (hasConnectionIssue) { + connectingStreakRef.current += 1; + } else { + connectingStreakRef.current = 0; + } + setHasSandboxError(connectingStreakRef.current >= 3); }; // Check immediately and then periodically checkForErrors(); const interval = setInterval(checkForErrors, 2000); // Check every 2 seconds - return () => clearInterval(interval); + return () => { + clearInterval(interval); + }; }, [branches.activeBranch, editorEngine.frames, editorEngine.branches]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧠 Learnings (2)
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/trpc/react.tsx : Keep tRPC React client provider(s) behind a client boundary (ensure this provider file is a client component)
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
packages/ui/src/components/sonner.tsx (1)
toast(19-19)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
1-1: Client component boundary correctly setGood catch adding 'use client' given state, events, and toasts.
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
Outdated
Show resolved
Hide resolved
| toast.error('Sandbox session not available'); | ||
| setRestarting(false); |
There was a problem hiding this comment.
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.
- Remove inefficient polling mechanism that ran every 2 seconds - Replace with MobX computed value that only updates on actual state changes - Remove useEffect and setInterval, eliminating unnecessary re-renders - Only check active branch's sandbox state instead of iterating all frames - Reduce memory usage and CPU cycles significantly - Error state now updates reactively when sandbox state changes This change eliminates: - Polling every 2 seconds - Iterating through all frames repeatedly - Unnecessary component re-renders - Memory allocation from repeated array operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove hasTimedOut check that doesn't exist on SessionManager type - Keep only isConnecting and isIndexing checks for error detection - Fixes Vercel build failure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
| const hasSandboxError = (() => { | ||
| const activeBranch = branches.activeBranch; | ||
| if (!activeBranch) return false; | ||
|
|
||
| // Only check the active branch's sandbox state (much more efficient) | ||
| const branchData = branches.getBranchDataById(activeBranch.id); | ||
| if (!branchData?.sandbox) return false; | ||
|
|
||
| // Check if sandbox is stuck connecting/indexing (indicates 502 or connection issue) | ||
| const isStuckConnecting = branchData.sandbox.session?.isConnecting || branchData.sandbox.isIndexing; | ||
|
|
||
| return isStuckConnecting; | ||
| })(); |
There was a problem hiding this comment.
Critical performance bug: The hasSandboxError computation is wrapped in an IIFE that executes on every render, defeating the purpose of MobX reactivity. This creates unnecessary re-computations and potential performance issues. The IIFE should be removed and this should be a direct computed value or moved to a useMemo hook with proper dependencies.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| 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); |
There was a problem hiding this comment.
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.
- Fix spinner visibility by using bg-accent/30 instead of opacity-50 during restart - Add 15-second grace period on startup before showing amber error state - Prevents amber warning on initial project load for better first impression - Button remains interactive but disabled, allowing spinner to be clearly visible - Error detection only activates after grace period expires 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (3)
194-195: Localize user‑facing strings (tooltips) per repo guidelines.Replace hardcoded text with next‑intl.
Illustrative changes:
- <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent> + <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent>Outside the selected range, add:
import { useTranslations } from 'next-intl'; const t = useTranslations('TerminalArea');Apply similarly for “Toggle Terminal”, and elsewhere in this file.
Also applies to: 250-251, 261-261
141-152: Leak: reload timer isn’t cleared on unmount; can set state on unmounted component.Store the timer id in a ref and clear it in a cleanup effect.
- // Wait 5 seconds before refreshing webviews to avoid 502 errors - setTimeout(() => { + // Wait 5 seconds before refreshing webviews to avoid 502 errors + reloadTimerRef.current = window.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); + }, 5000);Outside the selected range, add:
// near other refs const reloadTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); // cleanup useEffect(() => { return () => { if (reloadTimerRef.current) clearTimeout(reloadTimerRef.current); }; }, []);
118-161: Match PR intent: suppress toasts in expanded view via asilentoption.Currently both buttons toast; product notes say expanded view should not. Gate all toasts with
opts?.silent.- const handleRestartSandbox = async () => { + const handleRestartSandbox = async (opts?: { silent?: boolean }) => { const activeBranch = branches.activeBranch; if (!activeBranch || restarting) return; @@ const sandbox = branches.getSandboxById(activeBranch.id); if (!sandbox?.session) { - toast.error('Sandbox session not available'); + if (!opts?.silent) 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" />, - }); + if (!opts?.silent) { + toast.success('Sandbox restarted successfully', { + icon: <Icons.Cube className="h-4 w-4" />, + }); + } @@ } else { - toast.error('Failed to restart sandbox'); + if (!opts?.silent) 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'); + if (!opts?.silent) toast.error('An error occurred while restarting the sandbox'); setRestarting(false); } };
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
227-228: Call restart silently from expanded header.Aligns with “no toast in expanded view”.
- onClick={handleRestartSandbox} + onClick={() => handleRestartSandbox({ silent: true })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧠 Learnings (6)
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/trpc/react.tsx : Keep tRPC React client provider(s) behind a client boundary (ensure this provider file is a client component)
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.tsx : Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.tsx : Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.tsx : Avoid useMemo for creating MobX store instances
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (3)
packages/ui/src/components/sonner.tsx (1)
toast(19-19)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3620)packages/ui/src/components/tooltip.tsx (3)
Tooltip(72-72)TooltipTrigger(72-72)TooltipContent(72-72)
🔇 Additional comments (2)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (2)
1-1: Client component directive is correct.Required for event handlers and state.
171-173: Good: buttons disable during restart and without active branch.Prevents double submits and undefined session calls.
Consider adding an e2e test asserting the buttons are disabled while
restartingis true.Also applies to: 227-229
| // Track connection state and detect timeouts (actual 502 errors) | ||
| useEffect(() => { | ||
| const activeBranch = branches.activeBranch; | ||
| if (!activeBranch) { | ||
| setHasConnectionTimeout(false); | ||
| connectionStartTimeRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| const branchData = branches.getBranchDataById(activeBranch.id); | ||
| const isConnecting = branchData?.sandbox?.session?.isConnecting || branchData?.sandbox?.isIndexing; | ||
| const hasProvider = branchData?.sandbox?.session?.provider; | ||
|
|
||
| // If we have a provider, connection is successful - clear any timeout | ||
| if (hasProvider) { | ||
| setHasConnectionTimeout(false); | ||
| connectionStartTimeRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| // If not connecting, clear the timeout tracking | ||
| if (!isConnecting) { | ||
| setHasConnectionTimeout(false); | ||
| connectionStartTimeRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| // Start tracking connection time if not already | ||
| if (!connectionStartTimeRef.current) { | ||
| connectionStartTimeRef.current = Date.now(); | ||
| } | ||
|
|
||
| // Check if we've been connecting for too long (30 seconds like frame component) | ||
| const connectionDuration = Date.now() - connectionStartTimeRef.current; | ||
| const TIMEOUT_MS = 30000; // 30 seconds to match frame timeout | ||
|
|
||
| if (connectionDuration >= TIMEOUT_MS) { | ||
| // This is a real timeout/502 - show amber | ||
| setHasConnectionTimeout(true); | ||
| } else { | ||
| // Set up a timeout to check again | ||
| const remainingTime = TIMEOUT_MS - connectionDuration; | ||
| const timeoutId = setTimeout(() => { | ||
| // Re-check if still connecting after timeout | ||
| const stillConnecting = branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.isConnecting || | ||
| branches.getBranchDataById(activeBranch.id)?.sandbox?.isIndexing; | ||
| const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider; | ||
|
|
||
| if (stillConnecting && stillNoProvider) { | ||
| setHasConnectionTimeout(true); | ||
| } | ||
| }, remainingTime); | ||
|
|
||
| return () => clearTimeout(timeoutId); | ||
| } | ||
| }, [branches, branches.activeBranch]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
MobX reactivity gap: timeout effect won’t track session/provider changes reliably; can miss or stick error state.
The useEffect depends on branches and branches.activeBranch, but not on observable fields like session.isConnecting/provider. With MobX, these won’t retrigger the effect; the amber state can get stale. Use a MobX reaction to observe those fields and manage the timer; also keep/clear the timer via a ref.
Apply this diff to replace the effect:
- // Track connection state and detect timeouts (actual 502 errors)
- useEffect(() => {
- const activeBranch = branches.activeBranch;
- if (!activeBranch) {
- setHasConnectionTimeout(false);
- connectionStartTimeRef.current = null;
- return;
- }
-
- const branchData = branches.getBranchDataById(activeBranch.id);
- const isConnecting = branchData?.sandbox?.session?.isConnecting || branchData?.sandbox?.isIndexing;
- const hasProvider = branchData?.sandbox?.session?.provider;
-
- // If we have a provider, connection is successful - clear any timeout
- if (hasProvider) {
- setHasConnectionTimeout(false);
- connectionStartTimeRef.current = null;
- return;
- }
-
- // If not connecting, clear the timeout tracking
- if (!isConnecting) {
- setHasConnectionTimeout(false);
- connectionStartTimeRef.current = null;
- return;
- }
-
- // Start tracking connection time if not already
- if (!connectionStartTimeRef.current) {
- connectionStartTimeRef.current = Date.now();
- }
-
- // Check if we've been connecting for too long (30 seconds like frame component)
- const connectionDuration = Date.now() - connectionStartTimeRef.current;
- const TIMEOUT_MS = 30000; // 30 seconds to match frame timeout
-
- if (connectionDuration >= TIMEOUT_MS) {
- // This is a real timeout/502 - show amber
- setHasConnectionTimeout(true);
- } else {
- // Set up a timeout to check again
- const remainingTime = TIMEOUT_MS - connectionDuration;
- const timeoutId = setTimeout(() => {
- // Re-check if still connecting after timeout
- const stillConnecting = branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.isConnecting ||
- branches.getBranchDataById(activeBranch.id)?.sandbox?.isIndexing;
- const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider;
-
- if (stillConnecting && stillNoProvider) {
- setHasConnectionTimeout(true);
- }
- }, remainingTime);
-
- return () => clearTimeout(timeoutId);
- }
- }, [branches, branches.activeBranch]);
+ // Track connection state and detect timeouts (actual 502 errors) reactively
+ useEffect(() => {
+ const TIMEOUT_MS = 30000;
+ // Reset on branch change
+ setHasConnectionTimeout(false);
+ connectionStartTimeRef.current = null;
+ // React to observable changes
+ const dispose = reaction(
+ () => {
+ const id = branches.activeBranch?.id;
+ const data = id ? branches.getBranchDataById(id) : undefined;
+ return {
+ id,
+ isConnecting: Boolean(data?.sandbox?.session?.isConnecting || data?.sandbox?.isIndexing),
+ hasProvider: Boolean(data?.sandbox?.session?.provider),
+ };
+ },
+ ({ id, isConnecting, hasProvider }) => {
+ if (!id || hasProvider || !isConnecting) {
+ setHasConnectionTimeout(false);
+ connectionStartTimeRef.current = null;
+ if (timeoutIdRef.current) {
+ clearTimeout(timeoutIdRef.current);
+ timeoutIdRef.current = null;
+ }
+ return;
+ }
+ if (!connectionStartTimeRef.current) {
+ connectionStartTimeRef.current = Date.now();
+ }
+ const elapsed = Date.now() - connectionStartTimeRef.current;
+ const remaining = TIMEOUT_MS - elapsed;
+ if (remaining <= 0) {
+ setHasConnectionTimeout(true);
+ } else {
+ if (timeoutIdRef.current) clearTimeout(timeoutIdRef.current);
+ timeoutIdRef.current = setTimeout(() => {
+ const data = branches.getBranchDataById(id);
+ const stillConnecting = Boolean(data?.sandbox?.session?.isConnecting || data?.sandbox?.isIndexing);
+ const stillNoProvider = !Boolean(data?.sandbox?.session?.provider);
+ if (stillConnecting && stillNoProvider) setHasConnectionTimeout(true);
+ }, remaining);
+ }
+ },
+ { fireImmediately: true }
+ );
+ return () => {
+ if (timeoutIdRef.current) {
+ clearTimeout(timeoutIdRef.current);
+ timeoutIdRef.current = null;
+ }
+ dispose();
+ };
+ }, [branches.activeBranch?.id]);Outside the selected range, add:
// imports
import { reaction } from 'mobx';
// refs (near other refs)
const timeoutIdRef = useRef<ReturnType<typeof setTimeout> | null>(null);🤖 Prompt for AI Agents
In apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
around lines 57 to 112, the current useEffect only depends on branches and
branches.activeBranch so MobX observable changes to
session.isConnecting/session.provider won't reliably retrigger it; replace the
effect with a MobX reaction that observes the active branch's
sandbox.session.isConnecting, sandbox.session.provider and sandbox.isIndexing,
add import { reaction } from 'mobx', add a timeoutIdRef =
useRef<ReturnType<typeof setTimeout> | null>(null) near other refs, and
implement the reaction to start/clear a 30s timer (set hasConnectionTimeout when
exceeded), clear the timer on change and dispose the reaction on unmount so the
amber state neither gets stuck nor misses updates.
| const stillConnecting = branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.isConnecting || | ||
| branches.getBranchDataById(activeBranch.id)?.sandbox?.isIndexing; | ||
| const stillNoProvider = !branches.getBranchDataById(activeBranch.id)?.sandbox?.session?.provider; |
There was a problem hiding this comment.
Race condition bug: Multiple calls to branches.getBranchDataById(activeBranch.id) within the timeout callback can return different results if the branch data changes between calls. The activeBranch.id reference may also be stale after 5 seconds. Store the branch data in a variable at the start of the timeout callback to ensure consistent state checking.
| 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.
- Add mount time tracking to detect initial 502s - If no provider exists after 5 seconds from mount, show amber - Works even when sandbox isn't in "connecting" state - Handles immediate 502 failures on project load The key fix: Check for missing provider after grace period, not just when actively connecting 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| // Check if enough time has passed since mount (avoid false positives on initial load) | ||
| const timeSinceMount = Date.now() - mountTimeRef.current; | ||
| if (timeSinceMount < 5000) { |
There was a problem hiding this comment.
Consider using a named constant (e.g. GRACE_PERIOD_MS) instead of hardcoding 5000ms to improve readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
126-126: Localize user-facing text via next-intl.All hardcoded user-facing strings should use next-intl messages per the coding guidelines. This includes toast notifications and tooltip text.
Apply this diff to localize the strings:
+import { useTranslations } from 'next-intl'; export const TerminalArea = observer(({ children }: { children: React.ReactNode }) => { + const t = useTranslations('TerminalArea'); const editorEngine = useEditorEngine(); - toast.error('Sandbox session not available'); + toast.error(t('sandboxSessionUnavailable')); - toast.success('Sandbox restarted successfully', { + toast.success(t('restartSuccess'), { - toast.error('Failed to restart sandbox'); + toast.error(t('restartFailed')); - toast.error('An error occurred while restarting the sandbox'); + toast.error(t('restartErrorGeneric'));Also update the tooltip text:
- <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent> + <TooltipContent sideOffset={5} hideArrow>{t('restartSandbox')}</TooltipContent>Also applies to: 133-135, 150-150, 155-155
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (1)
61-113: Simplify the effect and fix potential timing issues.The current approach mixes timeout management with branch switching logic in a complex way. The timeout callback may access stale branch data, and the 5-second grace period logic could be more straightforward.
Apply this diff to simplify the error detection:
- // 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 + // Simplified error detection with grace period + useEffect(() => { + if (timeoutIdRef.current) { + clearTimeout(timeoutIdRef.current); + timeoutIdRef.current = null; + } + + const activeBranch = branches.activeBranch; + if (!activeBranch) { + setHasSandboxError(false); + return; + } + + // Set up grace period timer + const GRACE_PERIOD_MS = 5000; + timeoutIdRef.current = setTimeout(() => { + // Capture current state at timeout execution + const currentBranch = branches.activeBranch; + if (!currentBranch || currentBranch.id !== activeBranch.id) return; + + const branchData = branches.getBranchDataById(currentBranch.id); + const hasProvider = branchData?.sandbox?.session?.provider; + + if (!hasProvider) { + setHasSandboxError(true); + } + }, GRACE_PERIOD_MS); + + return () => { + if (timeoutIdRef.current) { + clearTimeout(timeoutIdRef.current); + timeoutIdRef.current = null; + } + }; + }, [branches.activeBranch?.id]); + + // Clear error state when provider is available + useEffect(() => { + const activeBranch = branches.activeBranch; + if (!activeBranch) return; + + const branchData = branches.getBranchDataById(activeBranch.id); + if (branchData?.sandbox?.session?.provider) { + setHasSandboxError(false); + } + }, [branches.activeBranch?.id, branches]);Define the constant at the top of the component:
+const GRACE_PERIOD_MS = 5000; + export const TerminalArea = observer(({ children }: { children: React.ReactNode }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧠 Learnings (5)
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/trpc/react.tsx : Keep tRPC React client provider(s) behind a client boundary (ensure this provider file is a client component)
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.tsx : Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
📚 Learning: 2025-09-14T01:44:21.199Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.199Z
Learning: Applies to apps/web/client/src/**/*.tsx : Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Applied to files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (2)
packages/ui/src/components/sonner.tsx (1)
toast(19-19)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3620)
🔇 Additional comments (6)
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx (6)
1-1: LGTM: 'use client' directive correctly placed.The component uses state, events, and toast APIs so it must be a Client Component, and the directive is properly positioned before all imports.
5-5: LGTM: Toast import correctly added.The toast import is necessary for the restart sandbox notifications and is correctly imported from the UI library.
11-11: LGTM: React hooks imported for new functionality.The additional hooks (
useEffect,useRef,useState) are correctly imported to support the new restart functionality and error detection.
53-59: LGTM: State management for restart flow is well-structured.The state variables are appropriately defined for tracking restart status, error detection, and timing. The
mountTimeRefprovides a clean baseline for avoiding false positives during startup.
165-192: LGTM: Restart button implementation in collapsed header is well-structured.The button properly handles disabled state, loading state with spinner, and error state styling. The tooltip and accessibility are correctly implemented.
221-248: LGTM: Restart button implementation in expanded header matches collapsed version.The implementation is consistent between collapsed and expanded states, maintaining the same styling, states, and behavior patterns.
| 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); | ||
| } 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
| } finally { | ||
| toast.dismiss(); | ||
| setRestarting(false); |
There was a problem hiding this comment.
The toast.dismiss() call in the finally block dismisses all active toasts, including any error toast that might have just been displayed. This could prevent users from seeing important error messages. Consider either:
- Moving
toast.dismiss()to only execute after successful operations - Adding a delay before dismissing to ensure error messages remain visible
- Using a more targeted approach by storing and dismissing only specific toast IDs
This would improve the user experience by ensuring error notifications remain visible long enough to be read.
| } finally { | |
| toast.dismiss(); | |
| setRestarting(false); | |
| } finally { | |
| // Only dismiss the loading toast if we stored its ID | |
| if (loadingToastId) { | |
| toast.dismiss(loadingToastId); | |
| } | |
| setRestarting(false); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| const editorEngine = useEditorEngine(); | ||
| const branches = editorEngine.branches; | ||
| const [restarting, setRestarting] = useState(false); | ||
| const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); |
There was a problem hiding this comment.
Consider using 'number' instead of 'NodeJS.Timeout' for the timeout ref in client-side code.
| const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); | |
| const timeoutIdRef = useRef<number | null>(null); |
| if (success) { | ||
| toast.loading('Restarting sandbox...'); | ||
| // Wait 5 seconds before refreshing webviews to avoid 502 errors | ||
| setTimeout(() => { |
There was a problem hiding this comment.
The setTimeout inside handleRestartSandbox isn’t cleared on unmount. Consider storing its ID in a ref and clearing it in a cleanup function to avoid potential memory leaks.
| console.error('Error restarting sandbox:', error); | ||
| toast.error('An error occurred while restarting the sandbox'); | ||
| } finally { | ||
| toast.dismiss(); |
There was a problem hiding this comment.
Calling toast.dismiss() in the finally block may dismiss notifications prematurely. Consider refining the toast management so that loading and success messages display appropriately.
| - Use Bun for all scripts; do not introduce npm/yarn. | ||
| - Avoid running the local dev server in automation contexts. | ||
| - Follow the project’s structure and client/server boundaries. | ||
| - Respect type safety and |
There was a problem hiding this comment.
The line "- Respect type safety and" appears to be incomplete. Please complete the thought or remove this line if it's not needed.
| - Respect type safety and |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
54-56: Method addition LGTM; consider reusing it internally.
getByBranchIdis correct and consistent with existing APIs. You can simplify internal call sites that manually filter bybranchIdto use this helper.Apply in-place refactor in
canDelete:- const branchId = selectedFrame.frame.branchId; - const framesInBranch = this.getAll().filter(frameData => frameData.frame.branchId === branchId); - if (framesInBranch.length <= 1) { + const branchId = selectedFrame.frame.branchId; + if (this.getByBranchId(branchId).length <= 1) { return false; // Cannot delete if this is the last frame in the branch }CLAUDE.md (2)
5-9: Restore canonical pointer to onlook/AGENTS.md.Long-term learnings require deferring to
onlook/AGENTS.md. Please add an explicit cross‑reference at the top so agents know which doc is authoritative on conflicts.Suggested edit:
### Purpose & Scope @@ -- Audience: automated coding agents working within this repository. +- Audience: automated coding agents working within this repository. +- Canonical policy: if guidance here conflicts with `onlook/AGENTS.md`, defer to `onlook/AGENTS.md`.
34-35: Incomplete bullet point.The list item ends abruptly (“Respect type safety and”). Complete the sentence to avoid ambiguity.
Suggested fix:
-- Respect type safety and +- Respect type safety and pass all lint/typecheck gates (no `any` unless unavoidable).apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx (3)
15-15: Browser timer typing + effect deps.
- In client code prefer
ReturnType<typeof setTimeout>overNodeJS.Timeout.- Dependency on the
branchesobject will retrigger the effect more than intended. Limit toactiveBranch?.id(and any explicit suppressors), matching the comment.Apply:
- const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); + const timeoutIdRef = useRef<ReturnType<typeof setTimeout> | null>(null); @@ - }, [branches.activeBranch?.id, branches]); // Only re-run when branch ID changes + }, [branches.activeBranch?.id, restarting]); // Only re-run when branch ID changes or restart state togglesAlso applies to: 64-65
19-44: Suppress amber error state during an intentional restart.While
restartingis true, avoid flippinghasSandboxErrorbased on provider absence to prevent confusing UI.Minimal guard:
useEffect(() => { + if (restarting) { + setHasSandboxError(false); + return; + }Also keep the existing cleanup logic.
Also applies to: 45-56
115-138: Add accessible label to the icon button.Provide
aria-labelfor screen readers; reuse the i18n tooltip text.- <button + <button onClick={handleRestartSandbox} disabled={disabled} + aria-label={t('sandbox.restartTooltip')} className={cn(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CLAUDE.md(1 hunks)apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx(4 hunks)apps/web/client/src/components/store/editor/frames/manager.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsxapps/web/client/src/components/store/editor/frames/manager.ts
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsxapps/web/client/src/components/store/editor/frames/manager.ts
🧠 Learnings (2)
📚 Learning: 2025-09-14T01:43:40.581Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-14T01:43:40.581Z
Learning: Read and follow onlook/AGENTS.md before making any change; do not proceed until its rules are internalized
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-14T01:43:40.581Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-14T01:43:40.581Z
Learning: If any instruction here conflicts with defaults, prefer onlook/AGENTS.md; when in doubt, re-read it before acting
Applied to files:
CLAUDE.md
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx (4)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)packages/ui/src/components/sonner.tsx (1)
toast(19-19)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3620)packages/ui/src/components/tooltip.tsx (3)
Tooltip(72-72)TooltipTrigger(72-72)TooltipContent(72-72)
🔇 Additional comments (2)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
5-5: Import move is fine.No behavior change; aligns with existing import grouping.
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx (1)
133-136: Style precedence is correct.Amber error state overrides loading/idle; matches the described behavior. No change needed.
apps/web/client/src/app/project/[id]/_components/bottom-bar/restart-sandbox-button.tsx
Show resolved
Hide resolved
| toast.error('Sandbox session not available'); | ||
| setRestarting(false); |
There was a problem hiding this comment.
Externalize user‑facing strings via next‑intl.
Avoid hardcoded UI text per repo guidelines. Use next-intl and add keys under apps/web/client/messages/*.
Apply this patch and I can follow up with message keys:
@@
-import { Icons } from '@onlook/ui/icons';
+import { Icons } from '@onlook/ui/icons';
+import { useTranslations } from 'next-intl';
@@
export const RestartSandboxButton = observer(({ className }: { className?: string }) => {
const editorEngine = useEditorEngine();
+ const t = useTranslations('toolbar'); // namespace suggestion
@@
- toast.error('Sandbox session not available');
+ toast.error(t('sandbox.sessionUnavailable'));
@@
- toast.loading('Restarting sandbox...');
+ const toastId = toast.loading(t('sandbox.restarting'));
@@
- toast.success('Sandbox restarted successfully', {
+ toast.success(t('sandbox.restarted'), {
icon: <Icons.Cube className="h-4 w-4" />,
+ id: toastId,
});
@@
- toast.error('Failed to restart sandbox');
+ toast.error(t('sandbox.restartFailed'));
@@
- toast.error('An error occurred while restarting the sandbox');
+ toast.error(t('sandbox.restartError'));
@@
- <TooltipContent sideOffset={5} hideArrow>Restart Sandbox</TooltipContent>
+ <TooltipContent sideOffset={5} hideArrow>{t('sandbox.restartTooltip')}</TooltipContent>Proposed keys:
- toolbar.sandbox.sessionUnavailable
- toolbar.sandbox.restarting
- toolbar.sandbox.restarted
- toolbar.sandbox.restartFailed
- toolbar.sandbox.restartError
- toolbar.sandbox.restartTooltip
Also applies to: 82-83, 93-95, 99-104, 140-141
|
|
||
| const success = await sandbox.session.restartDevServer(); | ||
| if (success) { | ||
| toast.loading('Restarting sandbox...'); |
There was a problem hiding this comment.
The toast.loading() call appears after the async operation has started but before the timeout. This creates a potential issue where the loading toast might remain visible if an error occurs during the timeout period. Consider moving this call before await sandbox.session.restartDevServer() and ensure toast.dismiss() is called in all code paths, including error scenarios. This would provide more consistent feedback to users regardless of operation outcome.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| if (restarting) { | ||
| return; | ||
| } | ||
| const activeBranch = branches.activeBranch; | ||
| if (!activeBranch) return; | ||
| if (restarting) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
There's a redundant check for the restarting state at lines 89-91 that duplicates the earlier check at lines 84-86. The second check can be safely removed to improve code clarity and avoid unnecessary conditional evaluation.
| if (restarting) { | |
| return; | |
| } | |
| const activeBranch = branches.activeBranch; | |
| if (!activeBranch) return; | |
| if (restarting) { | |
| return; | |
| } | |
| if (restarting) { | |
| return; | |
| } | |
| const activeBranch = branches.activeBranch; | |
| if (!activeBranch) return; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| } | ||
| const activeBranch = branches.activeBranch; | ||
| if (!activeBranch) return; | ||
| if (restarting) { |
There was a problem hiding this comment.
Redundant check for 'restarting' found. The flag is checked before and after fetching activeBranch, making the second check unnecessary.
Add a rotate icon button next to the terminal icon that allows users to restart the sandbox without using chat credits. The button is disabled when no branch is selected.
🤖 Generated with Claude Code
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Adds a "Restart Sandbox" button to the toolbar, updates UI with a new icon, and improves reliability by tracking connection states.
RestartSandboxButtoninrestart-sandbox-button.tsxto restart the sandbox without chat credits.RestartSandboxicon toicons/index.tsx.RestartSandboxButtonintoterminal-area.tsx.getByBranchId()toFramesManagerinframes/manager.tsto reload frames after restart.CLAUDE.mdwith a comprehensive repository agent and architecture guide.This description was created by
for 1638260. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Reliability
UI
Documentation