Conversation
…ice preview before there is an app.go
|
Caution Review failedThe pull request is closed. WalkthroughAdds a builder subsystem and wiring across backend and frontend. Backend: new pkg/buildercontroller implementing per-builder build/run lifecycle, output capture, status/versioning, registry, and Wave events; tsunami cache path helper and wave cache directory helpers; RPC surface and server handlers for builder actions. Frontend: new builder UI panels (preview, env, build output), singleton models, RPC client methods, and updated types. Misc: JSON tags on EditSpec, small util additions (MultiReaderLineBuffer.AddLine), import/path renames, and removal of an older frontend model file. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing focused review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/blockcontroller/tsunamicontroller.go (1)
256-263: Graceful stop not honored; process is killed first.Kill runs before stdin close, ignoring the graceful flag and TSUNAMI_CLOSEONSTDIN. Implement a graceful path (stdin close → wait → SIGTERM → kill fallback).
- if c.tsunamiProc.Cmd.Process != nil { - c.tsunamiProc.Cmd.Process.Kill() - } - - if c.tsunamiProc.StdinWriter != nil { - c.tsunamiProc.StdinWriter.Close() - } + // honor graceful shutdown + if graceful { + if c.tsunamiProc.StdinWriter != nil { + _ = c.tsunamiProc.StdinWriter.Close() + } + select { + case <-c.tsunamiProc.WaitCh: + // exited + case <-time.After(3 * time.Second): + if c.tsunamiProc.Cmd.Process != nil { + if runtime.GOOS != "windows" { + _ = c.tsunamiProc.Cmd.Process.Signal(syscall.SIGTERM) + select { + case <-c.tsunamiProc.WaitCh: + case <-time.After(2 * time.Second): + _ = c.tsunamiProc.Cmd.Process.Kill() + } + } else { + _ = c.tsunamiProc.Cmd.Process.Kill() + } + } + } + } else if c.tsunamiProc.Cmd.Process != nil { + _ = c.tsunamiProc.Cmd.Process.Kill() + } + if c.tsunamiProc.StdinWriter != nil { + _ = c.tsunamiProc.StdinWriter.Close() + }
🧹 Nitpick comments (27)
pkg/wps/wpstypes.go (1)
13-14: LGTM! New builder event constants are well-defined.The new event constants follow existing naming conventions and are appropriately placed near the related
Event_ControllerStatus. The string values correctly use lowercase format consistent with other events in the codebase.Optional improvements to consider:
Add brief documentation: While the current file has no comments on event constants, adding a comment block explaining the purpose of builder events would improve maintainability, especially as this is new functionality.
Consider alphabetical ordering: The constants could be alphabetically sorted to make them easier to locate as the list grows. Currently they're roughly grouped by feature, which is also reasonable.
Example documentation:
Event_ControllerStatus = "controllerstatus" + // Event_BuilderOutput is emitted when build output is generated + Event_BuilderOutput = "builderoutput" + // Event_BuilderStatus is emitted when builder status changes Event_BuilderStatus = "builderstatus" - Event_BuilderOutput = "builderoutput" Event_WaveObjUpdate = "waveobj:update"pkg/wstore/wstore_rtinfo.go (2)
47-74: Consider reducing duplication in map handling.The map conversion logic for
map[string]float64(lines 48-59) andmap[string]string(lines 61-72) follows nearly identical patterns. Consider extracting a generic helper or using type parameters to reduce duplication.Example refactor using a helper:
// Helper function to convert map[string]any to a typed map func convertMap[T any](inputMap map[string]any) map[string]T { outputMap := make(map[string]T) for k, v := range inputMap { if typedVal, ok := v.(T); ok { outputMap[k] = typedVal } } return outputMap } // Then in setFieldValue: if fieldValue.Kind() == reflect.Map { if fieldValue.Type().Key().Kind() == reflect.String && fieldValue.Type().Elem().Kind() == reflect.Float64 { if inputMap, ok := value.(map[string]any); ok { fieldValue.Set(reflect.ValueOf(convertMap[float64](inputMap))) } return } if fieldValue.Type().Key().Kind() == reflect.String && fieldValue.Type().Elem().Kind() == reflect.String { if inputMap, ok := value.(map[string]any); ok { fieldValue.Set(reflect.ValueOf(convertMap[string](inputMap))) } return } return }
19-19: Add documentation for the helper function.Consider adding a doc comment explaining
setFieldValue's behavior, particularly:
- That it silently ignores type mismatches
- Which types are supported
- That nil values reset fields to zero value
Example:
// setFieldValue sets fieldValue using value, performing type conversions as needed. // Supports: string, bool, int (from int/int64/float64), map[string]float64, // map[string]string, and interface{}. // Silently ignores unsupported types or conversion failures. // nil values reset the field to its zero value. func setFieldValue(fieldValue reflect.Value, value any) {pkg/buildercontroller/buildercontroller.go (1)
135-153: Consider using condition variable for build completion.The polling loop checks build status every 100ms. While acceptable for this use case, a condition variable would be more efficient.
Example pattern:
// Add to BuilderController: buildCond *sync.Cond // Initialize in GetOrCreateController: bc.buildCond = sync.NewCond(&bc.statusLock) // In setStatus_nolock, after updating status: bc.buildCond.Broadcast() // In waitForBuildDone: bc.statusLock.Lock() for bc.status == BuilderStatus_Building { if ctx.Err() != nil { bc.statusLock.Unlock() return ctx.Err() } bc.buildCond.Wait() } bc.statusLock.Unlock() return nilpkg/blockcontroller/tsunamicontroller.go (2)
98-102: Good centralization; avoid double-computing cache path.Using tsunamiutil is the right direction. Minor: isBuildCacheUpToDate() recomputes the same path; pass the already‑computed cachePath to avoid drift and repeated work.
- func isBuildCacheUpToDate(appPath string) (bool, error) { - appName := build.GetAppName(appPath) - osArch := runtime.GOOS + "-" + runtime.GOARCH - cachePath, err := tsunamiutil.GetTsunamiAppCachePath("local", appName, osArch) - if err != nil { - return false, err - } + func isBuildCacheUpToDate(appPath string, cachePath string) (bool, error) { // ... cacheInfo, err := os.Stat(cachePath) // ... } - upToDate, err := isBuildCacheUpToDate(appPath) + upToDate, err := isBuildCacheUpToDate(appPath, cachePath)Also applies to: 164-168
210-214: Bound the port-wait with a default timeout when ctx has no deadline.Prevents indefinite hangs if callers pass context.Background().
- tsunamiProc, err := runTsunamiAppBinary(ctx, cachePath, appPath, blockMeta) + // ensure bounded wait for port if caller did not set a deadline + timeoutCtx := ctx + cancel := func() {} + if _, ok := ctx.Deadline(); !ok { + timeoutCtx, cancel = context.WithTimeout(ctx, 30*time.Second) + } + defer cancel() + tsunamiProc, err := runTsunamiAppBinary(timeoutCtx, cachePath, appPath, blockMeta)pkg/tsunamiutil/tsunamiutil.go (1)
14-30: Solid utility; creates parent dir and handles Windows suffix.LGTM. Optionally, accept separate os/arch params to avoid relying on a string prefix check in callers, but current approach is fine.
frontend/builder/builder-buildpanel.tsx (2)
20-25: Only auto‑scroll when user is near the bottom.Prevents jumping when the user scrolls up to inspect logs.
- useEffect(() => { - if (scrollRef.current) { - scrollRef.current.scrollTop = scrollRef.current.scrollHeight; - } - }, [outputLines]); + useEffect(() => { + const el = scrollRef.current; + if (!el) return; + const nearBottom = el.scrollHeight - el.scrollTop - el.clientHeight < 32; + if (nearBottom) { + el.scrollTop = el.scrollHeight; + } + }, [outputLines]);
26-41: Cap rendered lines to avoid memory/render blowups.Long builds can freeze the UI. Clamp to last N lines (e.g., 5k) before join.
const BuilderBuildPanel = memo(() => { const model = BuilderBuildPanelModel.getInstance(); - const outputLines = useAtomValue(model.outputLines); + const outputLines = useAtomValue(model.outputLines); + const MAX_LINES = 5000; + const visibleLines = + outputLines.length > MAX_LINES ? outputLines.slice(-MAX_LINES) : outputLines; const scrollRef = useRef<HTMLDivElement>(null); … - {outputLines.length === 0 ? ( + {visibleLines.length === 0 ? ( <span className="text-secondary">Waiting for output...</span> ) : ( - outputLines.join("\n") + visibleLines.join("\n") )}frontend/builder/tabs/builder-envtab.tsx (4)
16-19: Remove unused state and avoid duplicate error UI.
builderIdisn’t used; drop it (and theatomsimport if nothing else needs it).- Env tab renders its own error banner while the panel already shows a global ErrorStrip; drop the local one to prevent duplicate messages.
Apply:
-import { atoms } from "@/store/global"; ... - const builderId = useAtomValue(atoms.builderId); - const envVarsObj = useAtomValue(model.envVarsAtom); - const error = useAtomValue(model.errorAtom); + const envVarsObj = useAtomValue(model.envVarsAtom); ... - {error && ( - <div className="mb-4 p-2 bg-red-500/20 text-red-500 rounded text-sm"> - {error} - </div> - )}Also applies to: 77-81
90-113: Use stable row keys; avoid index keys to prevent input glitches.Index keys can mis-map inputs after remove/insert. Use a stable
idper row and preserve it across syncs.Apply:
type EnvVar = { + id: string; name: string; value: string; };Initialize and preserve ids when syncing from the model:
-const [envVars, setEnvVars] = useState<EnvVar[]>([]); +const [envVars, setEnvVars] = useState<EnvVar[]>([]); useEffect(() => { - setEnvVars(Object.entries(envVarsObj).map(([name, value]) => ({ name, value }))); + setEnvVars((prev) => { + const byName = new Map(prev.map((v) => [v.name, v])); + return Object.entries(envVarsObj).map(([name, value]) => { + const existing = byName.get(name); + return existing ?? { id: crypto?.randomUUID?.() ?? `env_${Date.now()}_${Math.random().toString(36).slice(2)}`, name, value }; + }); + }); }, [envVarsObj]);When adding a row:
-const handleAddVar = useCallback(() => { - const newVars = [...envVars, { name: "", value: "" }]; +const handleAddVar = useCallback(() => { + const newVars = [...envVars, { id: crypto?.randomUUID?.() ?? `env_${Date.now()}_${Math.random().toString(36).slice(2)}`, name: "", value: "" }]; setEnvVars(newVars); }, [envVars]);Render with stable keys:
- envVars.map((envVar, index) => ( - <div key={index} className="flex items-center gap-2"> + envVars.map((envVar, index) => ( + <div key={envVar.id} className="flex items-center gap-2">Also applies to: 9-12, 20-21, 22-25, 36-40
26-35: Guard against duplicate/invalid variable names (optional).Currently, later entries overwrite earlier ones silently. Consider warning on duplicates and validating names (e.g., /^[A-Z_][A-Z0-9_]*$/) to avoid subtle config errors.
36-40: Focus the newly added row for faster entry (optional).After adding, focus the new row’s “Variable Name” input to optimize keyboard flow.
Example:
// Keep a ref map<id, HTMLInputElement>, then after setEnvVars(...) call refMap[newId]?.focus()pkg/wavebase/wavebase.go (2)
193-236: Prefer os.UserCacheDir for cross‑platform correctness.Leverage the standard library’s cache dir resolution, then fall back to per‑OS logic.
Apply:
func resolveWaveCachesDir() string { - var cacheDir string + var cacheDir string appBundle := "waveterm" if IsDevMode() { appBundle = "waveterm-dev" } - switch runtime.GOOS { + if ud, err := os.UserCacheDir(); err == nil && ud != "" { + return filepath.Join(ud, appBundle) + } + + switch runtime.GOOS { case "darwin": homeDir := GetHomeDir() cacheDir = filepath.Join(homeDir, "Library", "Caches", appBundle) case "linux": xdgCache := os.Getenv("XDG_CACHE_HOME") if xdgCache != "" { cacheDir = filepath.Join(xdgCache, appBundle) } else { homeDir := GetHomeDir() cacheDir = filepath.Join(homeDir, ".cache", appBundle) } case "windows": localAppData := os.Getenv("LOCALAPPDATA") if localAppData != "" { cacheDir = filepath.Join(localAppData, appBundle, "Cache") } }
72-74: Minor style: sync.Once by value.The idiomatic pattern is
var waveCachesDirOnce sync.Once(not a pointer). Current code works but can be simplified.-var waveCachesDirOnce = &sync.Once{} +var waveCachesDirOnce sync.Oncefrontend/builder/store/builder-buildpanel-model.ts (2)
45-49: Cap output buffer to avoid unbounded memory growth.Append with a max line window (e.g., 5–10k) to protect the tab.
Example:
- globalStore.set(this.outputLines, (prev) => [...prev, ...data.lines]); + const MAX_LINES = 10000; + globalStore.set(this.outputLines, (prev) => { + const next = prev.concat(data.lines!); + return next.length > MAX_LINES ? next.slice(next.length - MAX_LINES) : next; + });
65-71: Optional: clear output on dispose.If the panel is per‑builder session, consider
this.clearOutput()ondispose()to avoid showing stale logs when re‑opened.pkg/wshrpc/wshserver/wshserver.go (2)
979-989: Avoid creating controllers for read‑only queries (optional).
GetOrCreateControllerwill instantiate a controller even if one doesn’t exist. If you have aGetControlleraccessor, prefer it; otherwise this is acceptable.
991-994: Same validation for GetBuilderOutput.Add the same empty id guard as above.
func (ws *WshServer) GetBuilderOutputCommand(ctx context.Context, builderId string) ([]string, error) { + if strings.TrimSpace(builderId) == "" { + return nil, fmt.Errorf("invalid builderId") + } bc := buildercontroller.GetOrCreateController(builderId) return bc.GetOutput(), nil }frontend/builder/builder-apppanel.tsx (3)
110-113: Initialize when builderId is available.If
builderIdarrives after mount, initialization may be skipped. Re-run when it becomes available.Apply:
-useEffect(() => { - model.initialize(); -}, []); +useEffect(() => { + if (builderId) { + model.initialize(); + } +}, [builderId]);
254-254: Keep a single error surface.Since the panel renders a global
ErrorStrip, remove the Env tab’s local error banner to avoid duplicates. See env tab comment.
17-41: Optional: add a title to the status dot.Provide a tooltip with the current status for accessibility.
Example:
-return <span className={cn("w-2 h-2 rounded-full", statusDotColor)} />; +return <span className={cn("w-2 h-2 rounded-full", statusDotColor)} title={builderStatus?.status ?? "unknown"} />;frontend/builder/tabs/builder-previewtab.tsx (2)
67-75: Tie “Starting…” state to the actual start promise; remove fixed timeout.Avoid stale UI and unnecessary timer by awaiting onStart and resetting state in finally.
-const StoppedStateView = memo(({ onStart }: { onStart: () => void }) => { +const StoppedStateView = memo(({ onStart }: { onStart: () => Promise<void> }) => { const [isStarting, setIsStarting] = useState(false); - const handleStart = () => { - setIsStarting(true); - onStart(); - setTimeout(() => setIsStarting(false), 2000); - }; + const handleStart = async () => { + setIsStarting(true); + try { + await onStart(); + } finally { + setIsStarting(false); + } + };
132-134: Harden preview URL and force webview reloads when port/version change.Encode the client id and add a stable key to ensure Electron re-mounts on change.
- if (status === "stopped") { - return <StoppedStateView onStart={() => model.startBuilder()} />; - } + if (status === "stopped") { + return <StoppedStateView onStart={() => model.startBuilder()} />; + } - const shouldShowWebView = status === "running" && builderStatus?.port && builderStatus.port !== 0; + const shouldShowWebView = + status === "running" && typeof builderStatus?.port === "number" && builderStatus.port > 0; if (shouldShowWebView) { - const previewUrl = `http://localhost:${builderStatus.port}/?clientid=wave:${builderId}`; + const clientId = encodeURIComponent(`wave:${builderId ?? ""}`); + const previewUrl = `http://localhost:${builderStatus.port}/?clientid=${clientId}`; return ( <div className="w-full h-full"> - <webview src={previewUrl} className="w-full h-full" /> + <webview + key={`${builderStatus.port}-${builderStatus.version ?? 0}`} + src={previewUrl} + className="w-full h-full" + /> </div> ); }Also applies to: 138-145
pkg/wshrpc/wshclient/wshclient.go (1)
158-163: Client RPCs mirror server and existing patterns.Signatures and helper usage are correct. For large build logs, consider a streaming variant alongside GetBuilderOutputCommand to avoid big payloads.
Also applies to: 329-339, 577-582
frontend/builder/store/builder-apppanel-model.ts (2)
251-260: Allow re-initialization after dispose.Reset the initialized flag so initialize() can be called again after teardown (e.g., panel reopen).
dispose() { if (this.statusUnsubFn) { this.statusUnsubFn(); this.statusUnsubFn = null; } if (this.appGoUpdateUnsubFn) { this.appGoUpdateUnsubFn(); this.appGoUpdateUnsubFn = null; } + this.initialized = false; }
106-119: Surface env var load failures to the UI.Currently only logs to console; also set errorAtom so users see the problem.
} catch (err) { console.error("Failed to load environment variables:", err); + const msg = err && (err as any).message ? (err as any).message : String(err); + globalStore.set(this.errorAtom, `Failed to load environment variables: ${msg}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.roo/rules/rules.md(1 hunks)cmd/server/main-server.go(1 hunks)emain/emain-builder.ts(1 hunks)frontend/app/aipanel/waveai-model.tsx(1 hunks)frontend/app/store/wshclientapi.ts(3 hunks)frontend/builder/builder-apppanel.tsx(6 hunks)frontend/builder/builder-buildpanel.tsx(1 hunks)frontend/builder/builder-workspace.tsx(2 hunks)frontend/builder/store/builder-apppanel-model.ts(1 hunks)frontend/builder/store/builder-buildpanel-model.ts(1 hunks)frontend/builder/store/builderAppPanelModel.ts(0 hunks)frontend/builder/tabs/builder-codetab.tsx(2 hunks)frontend/builder/tabs/builder-envtab.tsx(1 hunks)frontend/builder/tabs/builder-previewtab.tsx(1 hunks)frontend/types/gotypes.d.ts(3 hunks)pkg/blockcontroller/tsunamicontroller.go(3 hunks)pkg/buildercontroller/buildercontroller.go(1 hunks)pkg/tsunamiutil/tsunamiutil.go(1 hunks)pkg/util/fileutil/fileutil.go(1 hunks)pkg/utilds/multireaderlinebuffer.go(1 hunks)pkg/wavebase/wavebase.go(2 hunks)pkg/waveobj/objrtinfo.go(1 hunks)pkg/wps/wpstypes.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(3 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)pkg/wstore/wstore_rtinfo.go(2 hunks)tsunami/build/build.go(30 hunks)tsunami/engine/render.go(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/builder/store/builderAppPanelModel.ts
🧰 Additional context used
🧬 Code graph analysis (17)
cmd/server/main-server.go (1)
pkg/wavebase/wavebase.go (1)
EnsureWaveCachesDir(234-236)
pkg/wshrpc/wshrpctypes.go (3)
frontend/app/store/wshclientapi.ts (4)
DeleteBuilderCommand(126-128)StartBuilderCommand(481-483)GetBuilderStatusCommand(276-278)GetBuilderOutputCommand(271-273)pkg/wshrpc/wshclient/wshclient.go (4)
DeleteBuilderCommand(159-162)StartBuilderCommand(578-581)GetBuilderStatusCommand(336-339)GetBuilderOutputCommand(330-333)pkg/buildercontroller/buildercontroller.go (1)
BuilderStatusData(441-447)
pkg/blockcontroller/tsunamicontroller.go (1)
pkg/tsunamiutil/tsunamiutil.go (1)
GetTsunamiAppCachePath(14-30)
tsunami/engine/render.go (1)
tsunami/vdom/vdom_types.go (4)
VDomFunc(25-32)ObjectType_Func(14-14)VDomRef(35-41)ObjectType_Ref(13-13)
frontend/builder/store/builder-buildpanel-model.ts (4)
frontend/app/store/global.ts (2)
atoms(816-816)WOS(866-866)frontend/app/store/wps.ts (1)
waveEventSubscribe(146-146)frontend/app/store/wshclientapi.ts (1)
RpcApi(592-592)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/app/store/wshclientapi.ts (2)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)pkg/wshrpc/wshrpctypes.go (3)
RpcOpts(323-329)BuilderStatusData(962-968)CommandStartBuilderData(958-960)
pkg/tsunamiutil/tsunamiutil.go (1)
pkg/wavebase/wavebase.go (2)
GetWaveCachesDir(227-232)TryMkdirs(255-271)
frontend/builder/builder-buildpanel.tsx (1)
frontend/builder/store/builder-buildpanel-model.ts (1)
BuilderBuildPanelModel(11-72)
frontend/builder/tabs/builder-envtab.tsx (1)
frontend/builder/store/builder-apppanel-model.ts (2)
BuilderAppPanelModel(14-261)setEnvVars(140-142)
pkg/wshrpc/wshserver/wshserver.go (6)
frontend/app/store/wshclientapi.ts (4)
DeleteBuilderCommand(126-128)StartBuilderCommand(481-483)GetBuilderStatusCommand(276-278)GetBuilderOutputCommand(271-273)pkg/wshrpc/wshclient/wshclient.go (4)
DeleteBuilderCommand(159-162)StartBuilderCommand(578-581)GetBuilderStatusCommand(336-339)GetBuilderOutputCommand(330-333)pkg/buildercontroller/buildercontroller.go (3)
DeleteController(81-96)GetOrCreateController(62-79)BuilderStatusData(441-447)pkg/wshrpc/wshrpctypes.go (2)
CommandStartBuilderData(958-960)BuilderStatusData(962-968)pkg/wstore/wstore_rtinfo.go (1)
GetRTInfo(128-138)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)
frontend/types/gotypes.d.ts (2)
pkg/buildercontroller/buildercontroller.go (1)
BuilderStatusData(441-447)pkg/wshrpc/wshrpctypes.go (2)
BuilderStatusData(962-968)CommandStartBuilderData(958-960)
frontend/builder/builder-apppanel.tsx (4)
frontend/builder/store/builder-apppanel-model.ts (2)
BuilderAppPanelModel(14-261)TabType(12-12)frontend/builder/store/builder-focusmanager.ts (1)
BuilderFocusManager(9-34)frontend/builder/utils/builder-focus-utils.ts (1)
builderAppHasSelection(44-59)frontend/app/element/errorboundary.tsx (1)
ErrorBoundary(6-33)
emain/emain-builder.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(592-592)emain/emain-wsh.ts (1)
ElectronWshClient(74-74)
frontend/builder/tabs/builder-previewtab.tsx (2)
frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(14-261)frontend/app/store/global.ts (1)
atoms(816-816)
pkg/wshrpc/wshclient/wshclient.go (2)
frontend/app/store/wshclientapi.ts (4)
DeleteBuilderCommand(126-128)GetBuilderOutputCommand(271-273)GetBuilderStatusCommand(276-278)StartBuilderCommand(481-483)pkg/wshrpc/wshrpctypes.go (3)
RpcOpts(323-329)BuilderStatusData(962-968)CommandStartBuilderData(958-960)
frontend/builder/store/builder-apppanel-model.ts (6)
pkg/wshrpc/wshrpctypes.go (1)
BuilderStatusData(962-968)frontend/app/store/global.ts (2)
atoms(816-816)WOS(866-866)frontend/app/store/wps.ts (1)
waveEventSubscribe(146-146)frontend/app/store/wshclientapi.ts (1)
RpcApi(592-592)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/util/util.ts (2)
base64ToString(460-460)stringToBase64(480-480)
pkg/buildercontroller/buildercontroller.go (8)
pkg/utilds/multireaderlinebuffer.go (2)
MultiReaderLineBuffer(9-15)MakeMultiReaderLineBuffer(17-27)pkg/wavebase/wavebase.go (3)
GetWaveCachesDir(227-232)TryMkdirs(255-271)GetWaveAppElectronExecPath(129-131)pkg/waveappstore/waveappstore.go (1)
GetAppDir(72-79)tsunami/build/build.go (6)
GetAppName(78-81)MakeOutputCapture(37-41)TsunamiBuildInternal(448-566)BuildOpts(65-76)OutputCapture(32-35)ParseTsunamiPort(861-884)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (3)
WaveEvent(25-31)Event_BuilderStatus(13-13)Event_BuilderOutput(14-14)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Builder(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (25)
tsunami/build/build.go (11)
32-63: LGTM! Clean and thread-safe implementation.The
OutputCapturetype is well-designed with proper mutex protection and nil-safe methods. The defensive copy inGetLinesprevents external mutation, and thePrintfmethod's fallback tolog.Printfensures graceful degradation when no capture is configured.
75-75: LGTM! Appropriate field addition.Adding
OutputCaptureas a pointer field enables optional output capture throughout the build pipeline while maintaining backward compatibility.
134-209: LGTM! Consistent OutputCapture integration.The OutputCapture is properly threaded through
verifyEnvironmentwith consistent usage patterns. All logging calls correctly useoc.Printf, which safely handles the nil case.
218-305: LGTM! Proper OutputCapture integration.The OutputCapture is correctly integrated throughout
createGoMod. Note that when verbose mode is enabled (lines 295-296), command output goes directly to Stdout/Stderr for real-time feedback, while build messages are captured. This design choice provides immediate visibility during verbose builds.
448-565: LGTM! Well-structured OutputCapture propagation.The OutputCapture is properly initialized and threaded through the entire build workflow, with consistent usage patterns. The capture is passed to helper functions (
copyFilesFromAppFS,copyScaffoldFS,moveFilesBack) to centralize all build output.
568-611: LGTM! Clean parameter addition.The
moveFilesBackfunction signature is appropriately updated to acceptOutputCapture, and all logging calls correctly useoc.Printffor consistent output capture.
613-664: LGTM! Consistent implementation.OutputCapture is properly integrated into
runGoBuild. The verbose command output (lines 647-648) continues to go directly to Stdout/Stderr for real-time visibility, while build progress messages are captured.
666-690: LGTM! Proper integration.OutputCapture is correctly used in
generateAppTailwindCssto capture build progress messages.
725-776: LGTM! Appropriate parameter threading.The
copyFilesFromAppFSfunction correctly accepts and uses OutputCapture to log file copy operations during the build process.
778-832: LGTM! Correct scoping of OutputCapture.OutputCapture is appropriately used for the build completion message, while the actual application runtime output goes to standard streams. This correctly separates build-time logging from runtime execution.
886-965: LGTM! Consistent implementation.The
copyScaffoldFSfunction properly uses OutputCapture to log scaffold copy operations, maintaining consistency with the overall build output capture design.pkg/wstore/wstore_rtinfo.go (1)
123-123: LGTM! Clean refactoring.Extracting the field assignment logic into
setFieldValueimproves readability and maintainability ofSetRTInfo. The delegation is clean and preserves the original behavior.pkg/util/fileutil/fileutil.go (1)
259-261: The review comment is incorrect—this is not a breaking change.The tool schema (lines 123–138 in
tools_builder.go) already defined the field names asold_strandnew_strin snake_case. External callers were already using these field names when invoking the tool. Adding explicit JSON tags to theEditSpecstruct aligns the Go implementation with the schema that was already published, rather than breaking it. This is a coordinated fix that ensures proper JSON marshaling/unmarshaling, not a breaking change to the external contract.Likely an incorrect or invalid review comment.
frontend/app/aipanel/waveai-model.tsx (1)
17-17: LGTM!Import path normalized to kebab-case convention.
pkg/waveobj/objrtinfo.go (1)
23-23: LGTM!Properly extends ObjRTInfo to support builder environment variables, following established patterns.
pkg/buildercontroller/buildercontroller.go (1)
389-401: Review status version increment behavior.The
statusVersionis incremented every timeGetStatus()is called, rather than when the status actually changes. This could lead to incorrect versioning if the same status is read multiple times.Consider incrementing the version only when status is modified in
setStatus_nolock():func (bc *BuilderController) GetStatus() BuilderStatusData { bc.statusLock.Lock() defer bc.statusLock.Unlock() - bc.statusVersion++ return BuilderStatusData{ Status: bc.status, Port: bc.port, ExitCode: bc.exitCode, ErrorMsg: bc.errorMsg, Version: bc.statusVersion, } }func (bc *BuilderController) setStatus_nolock(status string, port int, exitCode int, errorMsg string) { bc.statusLock.Lock() bc.status = status bc.port = port bc.exitCode = exitCode bc.errorMsg = errorMsg + bc.statusVersion++ bc.statusLock.Unlock() go bc.publishStatus() }Verify whether the current increment-on-read behavior is intentional for this use case.
emain/emain-builder.ts (1)
106-106: LGTM!Properly cleans up backend builder resources when window is closed using fire-and-forget RPC pattern.
cmd/server/main-server.go (1)
356-360: LGTM!Properly ensures wave caches directory exists during startup, following established patterns for other directory checks.
frontend/builder/builder-workspace.tsx (2)
8-9: LGTM!Import path normalized to kebab-case convention and new BuilderBuildPanel component added.
119-119: LGTM!Replaced placeholder with actual BuilderBuildPanel component.
frontend/builder/tabs/builder-codetab.tsx (2)
5-5: LGTM!Import path normalized to kebab-case convention.
58-58: LGTM!Changing
blockIdfrom empty string tonullis more semantically correct.frontend/types/gotypes.d.ts (1)
113-121: Type additions match Go; generated file OK.Also applies to: 347-351, 800-801
pkg/wshrpc/wshrpctypes.go (1)
162-166: Server wiring verified—handlers are correctly registered via reflection framework.The four builder RPC commands have complete end-to-end wiring:
- Constants defined (lines 162–165): deletebuilder, startbuilder, getbuilderstatus, getbuilderoutput
- Interface methods defined (lines 308–311): DeleteBuilderCommand, StartBuilderCommand, GetBuilderStatusCommand, GetBuilderOutputCommand
- Server implementations exist (wshserver.go lines 961–1001)
- Framework automatically derives command names from method names (wshrpcmeta.go): method "DeleteBuilderCommand" → command "deletebuilder" (via lines 74–75 logic: strip "Command" suffix, lowercase)
No manual routing needed—the reflection-based dispatch in wshrpcmeta.go handles all routing automatically.
frontend/app/store/wshclientapi.ts (1)
125-128: RPC additions look consistent and correct.Names and shapes match server-side commands; implementation follows existing pattern. LGTM.
Also applies to: 270-278, 480-483
| originalEnvVarsAtom: PrimitiveAtom<Record<string, string>> = atom<Record<string, string>>({}); | ||
| isLoadingAtom: PrimitiveAtom<boolean> = atom<boolean>(false); | ||
| errorAtom: PrimitiveAtom<string> = atom<string>(""); | ||
| builderStatusAtom = atom<BuilderStatusData>(null) as PrimitiveAtom<BuilderStatusData>; |
There was a problem hiding this comment.
Fix builderStatusAtom typing (nullable) to avoid type errors under strictNullChecks.
Initial value is null; the atom must be typed accordingly. Current cast can mask bugs.
- builderStatusAtom = atom<BuilderStatusData>(null) as PrimitiveAtom<BuilderStatusData>;
+ builderStatusAtom = atom<BuilderStatusData | null>(null) as PrimitiveAtom<BuilderStatusData | null>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builderStatusAtom = atom<BuilderStatusData>(null) as PrimitiveAtom<BuilderStatusData>; | |
| builderStatusAtom = atom<BuilderStatusData | null>(null) as PrimitiveAtom<BuilderStatusData | null>; |
🤖 Prompt for AI Agents
In frontend/builder/store/builder-apppanel-model.ts around line 24, the atom is
initialized with null but typed as BuilderStatusData, causing strictNullChecks
type errors and masking issues; change the atom typing to accept null (e.g.,
BuilderStatusData | null) and update the variable/atom declaration to
PrimitiveAtom<BuilderStatusData | null> (remove the unsafe cast) so the initial
null value is correctly typed and TypeScript enforces null handling.
| async initialize() { | ||
| if (this.initialized) return; | ||
| this.initialized = true; | ||
|
|
||
| const builderId = globalStore.get(atoms.builderId); | ||
| if (!builderId) return; | ||
|
|
There was a problem hiding this comment.
Don’t mark initialized before builderId exists.
If builderId isn’t set yet, the model becomes “initialized” and never subscribes later.
Apply:
async initialize() {
- if (this.initialized) return;
- this.initialized = true;
-
- const builderId = globalStore.get(atoms.builderId);
- if (!builderId) return;
+ if (this.initialized) return;
+ const builderId = globalStore.get(atoms.builderId);
+ if (!builderId) return; // try again later when builderId is set
+ this.initialized = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async initialize() { | |
| if (this.initialized) return; | |
| this.initialized = true; | |
| const builderId = globalStore.get(atoms.builderId); | |
| if (!builderId) return; | |
| async initialize() { | |
| if (this.initialized) return; | |
| const builderId = globalStore.get(atoms.builderId); | |
| if (!builderId) return; // try again later when builderId is set | |
| this.initialized = true; |
🤖 Prompt for AI Agents
In frontend/builder/store/builder-buildpanel-model.ts around lines 27-33, the
code sets this.initialized = true before checking builderId which causes the
model to be marked initialized even when builderId is missing and prevents later
subscription; move the initialization flag so it is set only after builderId is
confirmed (and after any subscriptions are established), i.e., check if
builderId exists first and return without setting this.initialized when it does
not, then set this.initialized = true after successful setup/subscription.
| import * as keyutil from "@/util/keyutil"; | ||
| import { useAtomValue } from "jotai"; | ||
| import { memo, useEffect } from "react"; | ||
| import { memo } from "react"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that model initialization handles file loading.
The useEffect hooks for loading the app file and subscribing to wave events have been removed. Ensure that BuilderAppPanelModel.getInstance() properly handles initial file loading and event subscriptions on its own lifecycle.
Run the following script to verify the model's initialization handles these concerns:
🏁 Script executed:
#!/bin/bash
# Check if BuilderAppPanelModel handles file loading and subscriptions in constructor or init method
ast-grep --pattern $'class BuilderAppPanelModel {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Also check for any init or load methods
rg -A10 "class BuilderAppPanelModel" frontend/builder/store/builder-apppanel-model.tsLength of output: 701
I need to gather more complete information about the BuilderAppPanelModel implementation to verify if it properly handles file loading and event subscriptions.
🏁 Script executed:
#!/bin/bash
# Get the full BuilderAppPanelModel class implementation
cat frontend/builder/store/builder-apppanel-model.tsLength of output: 9260
🏁 Script executed:
#!/bin/bash
# Search for file loading related methods
rg -n "loadAppFile|loadFile|load\(" frontend/builder/store/builder-apppanel-model.ts -A3Length of output: 442
🏁 Script executed:
#!/bin/bash
# Search for event subscription setup
rg -n "subscribeToWaveEvents|subscribe|addEventListener" frontend/builder/store/builder-apppanel-model.ts -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for constructor and getInstance methods
rg -n "constructor|getInstance" frontend/builder/store/builder-apppanel-model.ts -A5Length of output: 533
🏁 Script executed:
#!/bin/bash
# Check the original file (before changes) for useEffect hooks being removed
rg -n "useEffect" frontend/builder/tabs/builder-codetab.tsx -B2 -A5Length of output: 0
I need to verify whether the component is properly calling the model's initialization and cleanup methods.
🏁 Script executed:
#!/bin/bash
# Check how builder-codetab.tsx uses the BuilderAppPanelModel
cat frontend/builder/tabs/builder-codetab.tsxLength of output: 2171
🏁 Script executed:
#!/bin/bash
# Search for initialize or dispose calls in the component
rg -n "initialize\(\)|dispose\(\)|BuilderAppPanelModel" frontend/builder/tabs/builder-codetab.tsx -B2 -A2Length of output: 464
🏁 Script executed:
#!/bin/bash
# Check for useEffect in the entire builder directory
fd "builder-codetab" frontend/builder/tabs/ -x cat {}Length of output: 2171
Model initialization is not being called—model.initialize() must be invoked in a useEffect hook.
The BuilderAppPanelModel properly implements file loading and event subscriptions in its initialize() method (which calls loadAppFile(), loadEnvVars(), and sets up waveEventSubscribe handlers). However, the component removed the useEffect hooks but never added a call to model.initialize(). This means:
- The app file is never loaded
- Environment variables are never loaded
- Wave event subscriptions are never established
Add a useEffect hook to call model.initialize() and model.dispose() for proper lifecycle management:
useEffect(() => {
model.initialize();
return () => {
model.dispose();
};
}, []);🤖 Prompt for AI Agents
In frontend/builder/tabs/builder-codetab.tsx around line 9, the component only
imports memo and never calls model.initialize(), so the app file, env vars, and
wave event subscriptions are not set up; add a React useEffect hook that calls
model.initialize() on mount and returns a cleanup that calls model.dispose() on
unmount, and update the imports to include useEffect (i.e., import { memo,
useEffect } from "react"), ensuring the effect has an empty dependency array so
initialization runs once.
| func (mrlb *MultiReaderLineBuffer) AddLine(line string) { | ||
| mrlb.addLine(line) | ||
| mrlb.callLineCallback(line) | ||
| } |
There was a problem hiding this comment.
Callback ordering guarantee can be violated with concurrent calls.
The method calls addLine and callLineCallback without holding the lock across both operations. If AddLine is called concurrently from multiple goroutines, the callback invocations may not occur in the same order that lines were added to the buffer, violating the "guaranteed to run in order" promise documented on line 30.
Example race scenario:
- Goroutine A:
AddLine("line1")→addLine("line1")completes - Goroutine B:
AddLine("line2")→addLine("line2")completes - Goroutine B:
callLineCallback("line2")acquires lock first - Goroutine A:
callLineCallback("line1")waits
Result: callback for line2 executes before line1.
Consider one of these solutions:
Solution 1: Document the limitation
If concurrent calls to AddLine are not expected, add a comment:
+// AddLine appends a line and triggers the registered callback.
+// NOTE: If called concurrently, callbacks may not be invoked in the order lines were added.
+// For guaranteed ordering, serialize calls to AddLine or use from a single goroutine.
func (mrlb *MultiReaderLineBuffer) AddLine(line string) {
mrlb.addLine(line)
mrlb.callLineCallback(line)
}Solution 2: Ensure atomic add-and-callback
Refactor to hold the lock across both operations:
func (mrlb *MultiReaderLineBuffer) AddLine(line string) {
- mrlb.addLine(line)
- mrlb.callLineCallback(line)
+ mrlb.lock.Lock()
+ defer mrlb.lock.Unlock()
+
+ // Add line to buffer
+ mrlb.totalLineCount++
+ if len(mrlb.lines) >= mrlb.maxLines {
+ mrlb.lines = append(mrlb.lines[1:], line)
+ } else {
+ mrlb.lines = append(mrlb.lines, line)
+ }
+
+ // Call callback
+ if mrlb.lineCallback != nil {
+ mrlb.lineCallback(line)
+ }
}Note: This duplicates logic from addLine/callLineCallback, so you may want to refactor those methods to have lock-free internal versions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (mrlb *MultiReaderLineBuffer) AddLine(line string) { | |
| mrlb.addLine(line) | |
| mrlb.callLineCallback(line) | |
| } | |
| func (mrlb *MultiReaderLineBuffer) AddLine(line string) { | |
| mrlb.lock.Lock() | |
| defer mrlb.lock.Unlock() | |
| // Add line to buffer | |
| mrlb.totalLineCount++ | |
| if len(mrlb.lines) >= mrlb.maxLines { | |
| mrlb.lines = append(mrlb.lines[1:], line) | |
| } else { | |
| mrlb.lines = append(mrlb.lines, line) | |
| } | |
| // Call callback | |
| if mrlb.lineCallback != nil { | |
| mrlb.lineCallback(line) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/utilds/multireaderlinebuffer.go around lines 56-59 the AddLine method
releases the lock between addLine and callLineCallback which allows concurrent
AddLine calls to invoke callbacks out-of-order; fix by making the
add-and-callback atomic: acquire the same mutex, perform the add and then invoke
the callback while still holding the lock (or refactor to a private
addLineAndInvokeCallback helper that performs both operations under the lock),
or alternatively document that AddLine is not safe for concurrent callers if you
prefer not to change behavior; if you choose the locked approach, avoid
duplicating logic by extracting lock-free internal helpers used by both code
paths.
| if fieldValue.Kind() == reflect.Int { | ||
| switch v := value.(type) { | ||
| case int: | ||
| fieldValue.SetInt(int64(v)) | ||
| case int64: | ||
| fieldValue.SetInt(v) | ||
| case float64: | ||
| fieldValue.SetInt(int64(v)) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Float-to-int conversion truncates decimals.
Line 42 converts float64 to int64, which silently truncates any fractional part. For example, 1.9 becomes 1. This could cause unexpected data loss if non-integer values are stored in the info map.
Consider whether this is the intended behavior for your use case. If so, document it. If not, consider validation or logging when truncation occurs:
if fieldValue.Kind() == reflect.Int {
switch v := value.(type) {
case int:
fieldValue.SetInt(int64(v))
case int64:
fieldValue.SetInt(v)
case float64:
+ // Truncates fractional part
fieldValue.SetInt(int64(v))
}
return
}🤖 Prompt for AI Agents
In pkg/wstore/wstore_rtinfo.go around lines 35 to 45, the float64-to-int
conversion silently truncates fractional parts (e.g. 1.9 -> 1). Update the
float64 handling to detect non-zero fractional parts and handle them explicitly:
either reject and return/log an error indicating loss of precision, or apply an
explicit rounding strategy (e.g., math.Round) and document that choice.
Implement the check before calling SetInt (compare v to float64(int64(v)) or use
math.Modf), and ensure the function returns or logs an error when truncation
would occur if you choose to reject; otherwise apply math.Round and then SetInt,
and add a comment documenting the behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/builder/store/builder-apppanel-model.ts (1)
25-25: Previously flagged: Fix builderStatusAtom typing to include null.This issue was already identified in a previous review. The atom is initialized with
nullbut typed asBuilderStatusData, which causes type safety issues under strict TypeScript settings.
🧹 Nitpick comments (3)
frontend/builder/store/builder-apppanel-model.ts (3)
29-29: Replaceanytype with proper Monaco editor type.Using
anydefeats TypeScript's type safety. Consider importing the proper type from the Monaco editor package (e.g.,monaco.editor.IStandaloneCodeEditoror similar).- monacoEditorRef: { current: any | null } = { current: null }; + monacoEditorRef: { current: monaco.editor.IStandaloneCodeEditor | null } = { current: null };
140-140: Use proper error type handling in catch blocks.TypeScript types caught errors as
unknown. Accessingerr.messagewithout type checking can fail if the thrown value isn't an Error object. This pattern appears in multiple catch blocks throughout the file (lines 140, 156, 168, 199, 219).Example fix for line 140:
} catch (err) { console.error("Failed to save environment variables:", err); - globalStore.set(this.errorAtom, `Failed to save environment variables: ${err.message || "Unknown error"}`); + const errMsg = err instanceof Error ? err.message : "Unknown error"; + globalStore.set(this.errorAtom, `Failed to save environment variables: ${errMsg}`); }Apply similar pattern to other catch blocks at lines 156, 168, 199, and 219.
244-254: Consider resetting singleton instance in dispose.The
disposemethod cleans up subscriptions but doesn't reset the singleton instance. If the builder panel is closed and reopened, the same instance with potentially stale state will be reused. Consider addingBuilderAppPanelModel.instance = null;or providing a reset method to allow fresh initialization.dispose() { if (this.statusUnsubFn) { this.statusUnsubFn(); this.statusUnsubFn = null; } if (this.appGoUpdateUnsubFn) { this.appGoUpdateUnsubFn(); this.appGoUpdateUnsubFn = null; } this.debouncedRestart.cancel(); + this.initialized = false; + BuilderAppPanelModel.instance = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.roo/rules/rules.md(1 hunks)frontend/builder/store/builder-apppanel-model.ts(1 hunks)frontend/builder/tabs/builder-envtab.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/builder/tabs/builder-envtab.tsx
- .roo/rules/rules.md
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/builder/store/builder-apppanel-model.ts (7)
pkg/buildercontroller/buildercontroller.go (1)
BuilderStatusData(441-447)pkg/wshrpc/wshrpctypes.go (1)
BuilderStatusData(962-968)frontend/app/store/global.ts (2)
atoms(816-816)WOS(866-866)frontend/app/store/wps.ts (1)
waveEventSubscribe(146-146)frontend/app/store/wshclientapi.ts (1)
RpcApi(592-592)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/util/util.ts (2)
base64ToString(460-460)stringToBase64(480-480)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
| const currentStatus = globalStore.get(this.builderStatusAtom); | ||
| if (!currentStatus || !currentStatus.version || status.version > currentStatus.version) { | ||
| globalStore.set(this.builderStatusAtom, status); |
There was a problem hiding this comment.
Fix version comparison to handle version 0 correctly.
The condition !currentStatus.version is truthy when version is 0, which is a valid version number. This logic bug could cause stale status data to overwrite newer status if the new status has version 0.
- if (!currentStatus || !currentStatus.version || status.version > currentStatus.version) {
+ if (!currentStatus || currentStatus.version == null || status.version > currentStatus.version) {
globalStore.set(this.builderStatusAtom, status);
}🤖 Prompt for AI Agents
In frontend/builder/store/builder-apppanel-model.ts around lines 84 to 86, the
version check uses a falsy test (!currentStatus.version) which treats 0 as
missing; change the logic to explicitly check for undefined/null (e.g.,
currentStatus.version == null or typeof currentStatus.version === 'undefined')
before comparing versions so that version 0 is treated as valid, then perform
the numeric comparison (status.version > currentStatus.version) as before and
only overwrite when appropriate.
| async restartBuilder() { | ||
| const builderId = globalStore.get(atoms.builderId); | ||
| try { | ||
| await RpcApi.ControllerStopCommand(TabRpcClient, builderId); | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| await this.startBuilder(); | ||
| } catch (err) { | ||
| console.error("Failed to restart builder:", err); | ||
| globalStore.set(this.errorAtom, `Failed to restart builder: ${err.message || "Unknown error"}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace hard-coded delay with status polling.
The 500ms fixed delay between stop and start is fragile. If the builder takes longer to stop, the restart will fail or behave unpredictably. Consider polling the builder status or using a status-based wait mechanism.
Example approach:
async restartBuilder() {
const builderId = globalStore.get(atoms.builderId);
try {
await RpcApi.ControllerStopCommand(TabRpcClient, builderId);
// Poll status until stopped or timeout
for (let i = 0; i < 10; i++) {
await new Promise((resolve) => setTimeout(resolve, 200));
const status = await RpcApi.GetBuilderStatusCommand(TabRpcClient, builderId);
if (status.status === "stopped" || status.status === "error") {
break;
}
}
await this.startBuilder();
} catch (err) {
console.error("Failed to restart builder:", err);
const errMsg = err instanceof Error ? err.message : "Unknown error";
globalStore.set(this.errorAtom, `Failed to restart builder: ${errMsg}`);
}
}🤖 Prompt for AI Agents
In frontend/builder/store/builder-apppanel-model.ts around lines 160-170,
replace the fixed 500ms delay after ControllerStopCommand with a status-polling
loop that repeatedly calls RpcApi.GetBuilderStatusCommand(TabRpcClient,
builderId) at a short interval (e.g., 200ms) up to a reasonable timeout or max
attempts, breaking when status indicates "stopped" or "error"; after the loop
call this.startBuilder(); ensure errors are logged and the errorAtom is set
using err instanceof Error ? err.message : "Unknown error".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/builder/builder-apppanel.tsx (1)
203-211: Consider removing or commenting dead code.The Static Files tab is wrapped in
{false && ...}, making it unreachable dead code. Given this is a checkpoint PR, this may be intentional for future work.For cleaner code, consider either:
- Removing the dead code entirely if not needed soon
- Replacing with a TODO comment if planning to implement it later
- Using a feature flag constant instead of a boolean literal
-{false && ( - <TabButton - label="Static Files" - tabType="files" - isActive={activeTab === "files"} - isAppFocused={isAppFocused} - onClick={() => handleTabClick("files")} - /> -)} +{/* TODO: Implement Static Files tab */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/builder/builder-apppanel.tsx(6 hunks)frontend/builder/store/builder-apppanel-model.ts(1 hunks)frontend/builder/tabs/builder-envtab.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/builder/store/builder-apppanel-model.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/builder/builder-apppanel.tsx (4)
frontend/builder/store/builder-apppanel-model.ts (2)
BuilderAppPanelModel(21-295)TabType(13-13)frontend/builder/store/builder-focusmanager.ts (1)
BuilderFocusManager(9-34)frontend/builder/utils/builder-focus-utils.ts (1)
builderAppHasSelection(44-59)frontend/app/element/errorboundary.tsx (1)
ErrorBoundary(6-33)
frontend/builder/tabs/builder-envtab.tsx (1)
frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(21-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/builder/builder-apppanel.tsx (1)
171-171: Verify Tailwind v4 border-width syntax.The
border-b-3utility may not be valid in Tailwind CSS v4. The default border-width scale is0, 1, 2, 4, 8, and arbitrary values require bracket syntax.Apply this diff to use the correct arbitrary value syntax:
-className="w-full h-full flex flex-col border-b-3 border-border shadow-[0_2px_4px_rgba(0,0,0,0.1)]" +className="w-full h-full flex flex-col border-b-[3px] border-border shadow-[0_2px_4px_rgba(0,0,0,0.1)]"Alternatively, use a standard scale value like
border-b-2orborder-b-4if 3px is not critical.
| No environment variables defined. Click "Add Variable" to create one. | ||
| </div> | ||
| ) : ( | ||
| envVars.map((_, index) => <EnvVarRow key={index} model={model} index={index} />) |
There was a problem hiding this comment.
Use a stable key instead of array index.
Using array index as the key can cause React reconciliation issues when environment variables are removed. When an item is removed, all subsequent items shift indices, potentially causing React to incorrectly reuse component instances, leading to focus loss, stale data display, or animation glitches.
Consider adding a stable identifier to the EnvVar type (e.g., a generated ID) or use a composite key. For example:
-envVars.map((_, index) => <EnvVarRow key={index} model={model} index={index} />)
+envVars.map((envVar, index) => <EnvVarRow key={`${envVar.name}-${index}`} model={model} index={index} />)Note: Using name + index as a composite key is better than index alone, though a generated unique ID would be most robust if variables can be reordered in the future.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| envVars.map((_, index) => <EnvVarRow key={index} model={model} index={index} />) | |
| envVars.map((envVar, index) => <EnvVarRow key={`${envVar.name}-${index}`} model={model} index={index} />) |
🤖 Prompt for AI Agents
In frontend/builder/tabs/builder-envtab.tsx around line 93, the list is using
the array index as the React key which causes reconciliation bugs when items are
removed; add a stable identifier to each environment variable and use it as the
key (or at minimum use a composite key like `${env.name}-${index}` until you add
IDs). Update the EnvVar type to include an `id` (e.g., UUID or incrementing
string), ensure new rows are created with that id wherever env vars are added,
and change the map to pass `key={env.id}` to EnvVarRow so removals/reorders
won't shift keys and break component state.
1 participant
Got preview hooked up, log output hooked up, environment tab hooked up... the controller, restarting the apps. it is actually working! still lots to do and lots of hard coding to fix, but it is coming together...