waveapps builder window (scaffolding, restructure AI panel to work in both builder and tab windows)#2482
waveapps builder window (scaffolding, restructure AI panel to work in both builder and tab windows)#2482
Conversation
…al keys. implement Cmd:w
…more than just blocks...
…(and conn cache) into it as well
…t builderFocusManager...
WalkthroughThis PR implements Tsunami AI Builder V1: adds an Electron builder window manager and comprehensive IPC handlers; introduces a new frontend Builder app (workspace, panels, Preview/Files/Code tabs, focus managers, builder model) and wiring for builder init/route handling; migrates tab-scoped stores and WebSocket routing to generic oref/routeId APIs; adds a local/draft wave app store with publish/revert and file operations (including ReplaceInFile and recursive ReadDir); extends RTInfo/types for builder layout and chat; adds Go builder tools and integrates builder flows into app startup and runtime. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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)
⏰ 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)
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/app/store/global.ts (1)
61-67: Fix type safety:activetabidcannot beundefinedbuttabIdis optional.Line 64 assigns
initOpts.tabId(optional) touiContext.activetabid(required string). For builder windows wheretabIdis undefined, this creates a type violation. Either:
- Make
UIContext.activetabidoptional in the type definition, or- Provide a fallback value when
tabIdis missing.Downstream code in
emain-window.ts:681already has defensive checks (if (workspace.activetabid)), suggesting this undefined case is known but not properly typed.emain/emain-window.ts (2)
497-501: removeTabViewLater ignores delayMs parameterHardcoded 1000 ms defeats the parameter and surprises callers.
Apply this diff:
private removeTabViewLater(tabId: string, delayMs: number) { - setTimeout(() => { - this.removeTabView(tabId, false); - }, 1000); + setTimeout(() => { + this.removeTabView(tabId, false); + }, delayMs); }
651-653: Incorrect use ofthisin free function log
this.waveWindowIdis undefined here and logs “undefined”.Use the returned window id or drop it:
if (!newWin) { - console.log("error creating new window", this.waveWindowId); + console.log("error creating new window"); }
🧹 Nitpick comments (8)
aiprompts/tsunami-builder.md (1)
1-260: LGTM! Comprehensive design document.The architecture document clearly defines the V1 scope and aligns well with the implementation seen in other files (e.g.,
tools_builder.go).Optional: Address markdown linting hints.
Static analysis identified minor formatting issues:
- Missing periods after abbreviations (line 18: "etc.")
- Missing language specifiers on code blocks (lines 48, 135, 159, 214)
- Emphasis used instead of headings (lines 68, 86, 95, 102)
These are style improvements and can be addressed at your discretion.
Based on learnings
frontend/builder/builder-workspace.tsx (2)
25-51: Prevent race on builderId change by cancelling stale loadWithout a cancellation guard, a slow response can overwrite newer state.
Apply this diff:
- useEffect(() => { - const loadLayout = async () => { + useEffect(() => { + let cancelled = false; + const loadLayout = async () => { if (!builderId) { - setLayout(DEFAULT_LAYOUT); - setIsLoading(false); + if (!cancelled) { + setLayout(DEFAULT_LAYOUT); + setIsLoading(false); + } return; } try { const rtInfo = await RpcApi.GetRTInfoCommand(TabRpcClient, { oref: `builder:${builderId}`, }); - if (rtInfo?.["builder:layout"]) { - setLayout(rtInfo["builder:layout"] as Record<string, number>); - } else { - setLayout(DEFAULT_LAYOUT); - } + if (!cancelled) { + if (rtInfo?.["builder:layout"]) { + setLayout(rtInfo["builder:layout"] as Record<string, number>); + } else { + setLayout(DEFAULT_LAYOUT); + } + } } catch (error) { console.error("Failed to load builder layout:", error); - setLayout(DEFAULT_LAYOUT); + if (!cancelled) setLayout(DEFAULT_LAYOUT); } finally { - setIsLoading(false); + if (!cancelled) setIsLoading(false); } }; loadLayout(); - }, [builderId]); + return () => { + cancelled = true; + }; + }, [builderId]);
53-67: Cancel debounced save on unmount to avoid stale RPCsDebounced calls can fire after unmount or builderId change.
Apply this diff:
- const saveLayout = useCallback( - debounce(500, (newLayout: Record<string, number>) => { + const saveLayout = useMemo( + () => + debounce(500, (newLayout: Record<string, number>) => { if (!builderId) return; RpcApi.SetRTInfoCommand(TabRpcClient, { oref: `builder:${builderId}`, data: { "builder:layout": newLayout, }, }).catch((error) => { console.error("Failed to save builder layout:", error); }); - }), - [builderId] + }), + [builderId] ); + + useEffect(() => { + return () => { + (saveLayout as any)?.cancel?.(); + }; + }, [saveLayout]);frontend/builder/store/builderFocusManager.ts (1)
12-13: Make atom reference readonly to prevent accidental reassignmentAvoids accidental rebind of the atom.
Apply this diff:
- focusType: PrimitiveAtom<BuilderFocusType> = atom("app"); + readonly focusType: PrimitiveAtom<BuilderFocusType> = atom<BuilderFocusType>("app");frontend/wave.ts (1)
220-236: Type catch param for TS strict mode compatibilityIf useUnknownInCatchVariables is enabled, e.message/e.stack are errors. Annotate the catch param.
Apply this diff:
-async function initBuilderWrap(initOpts: BuilderInitOpts) { +async function initBuilderWrap(initOpts: BuilderInitOpts) { try { if (savedBuilderInitOpts) { await reinitBuilder(); return; } savedBuilderInitOpts = initOpts; await initBuilder(initOpts); - } catch (e) { + } catch (e: any) { getApi().sendLog("Error in initBuilder " + e.message + "\n" + e.stack); console.error("Error in initBuilder", e); } finally {Consider doing the same for initWaveWrap for consistency.
emain/emain-window.ts (1)
753-756: Remove unusedworkspaceHasWindowComputed but never used.
Apply this diff:
- const workspaceHasWindow = !!workspaceList.find((wse) => wse.workspaceid === workspaceId)?.windowid; - const choice = dialog.showMessageBoxSync(this, {emain/emain-ipc.ts (1)
342-352: More precise tilde expansion for native pathsCurrent replace() changes only first “~” anywhere; anchor it to path start.
Apply:
- filePath = filePath.replace("~", electronApp.getPath("home")); + filePath = filePath.replace(/^~(?=$|[\\/])/, electronApp.getPath("home"));emain/emain-builder.ts (1)
44-62: Consider hardening BrowserWindow webPreferencesExplicitly set security-related flags to avoid regressions if Electron defaults change.
Example:
const builderWindow = new BrowserWindow({ @@ webPreferences: { preload: path.join(getElectronAppBasePath(), "preload", "index.cjs"), - webviewTag: true, + webviewTag: true, + contextIsolation: true, + nodeIntegration: false, + sandbox: true, // verify builder flows still work before enabling }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
aiprompts/tsunami-builder.md(1 hunks)emain/emain-builder.ts(1 hunks)emain/emain-ipc.ts(1 hunks)emain/emain-log.ts(1 hunks)emain/emain-menu.ts(5 hunks)emain/emain-tabview.ts(1 hunks)emain/emain-wavesrv.ts(1 hunks)emain/emain-window.ts(8 hunks)emain/emain-wsh.ts(1 hunks)emain/emain.ts(5 hunks)emain/launchsettings.ts(1 hunks)emain/preload.ts(1 hunks)frontend/app/aipanel/aifeedbackbuttons.tsx(3 hunks)frontend/app/aipanel/aipanel.tsx(6 hunks)frontend/app/aipanel/aipanelheader.tsx(2 hunks)frontend/app/aipanel/aipanelinput.tsx(4 hunks)frontend/app/aipanel/aipanelmessages.tsx(1 hunks)frontend/app/aipanel/aitooluse.tsx(5 hunks)frontend/app/aipanel/waveai-model.tsx(10 hunks)frontend/app/store/focusManager.ts(2 hunks)frontend/app/store/global.ts(7 hunks)frontend/app/store/keymodel.ts(7 hunks)frontend/app/store/ws.ts(4 hunks)frontend/app/store/wshrouter.ts(2 hunks)frontend/app/store/wshrpcutil.ts(1 hunks)frontend/app/workspace/workspace-layout-model.ts(2 hunks)frontend/builder/builder-app.tsx(1 hunks)frontend/builder/builder-apppanel.tsx(1 hunks)frontend/builder/builder-workspace.tsx(1 hunks)frontend/builder/store/builderFocusManager.ts(1 hunks)frontend/builder/tabs/builder-codetab.tsx(1 hunks)frontend/builder/tabs/builder-filestab.tsx(1 hunks)frontend/builder/tabs/builder-previewtab.tsx(1 hunks)frontend/layout/lib/layoutModel.ts(4 hunks)frontend/types/custom.d.ts(3 hunks)frontend/types/gotypes.d.ts(2 hunks)frontend/wave.ts(5 hunks)pkg/aiusechat/tools_builder.go(1 hunks)pkg/aiusechat/usechat.go(1 hunks)pkg/eventbus/eventbus.go(2 hunks)pkg/util/fileutil/fileutil.go(2 hunks)pkg/util/fileutil/readdir.go(1 hunks)pkg/waveappstore/waveappstore.go(1 hunks)pkg/waveobj/objrtinfo.go(1 hunks)pkg/waveobj/wtype.go(2 hunks)pkg/web/ws.go(2 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wshutil/wshrouter.go(2 hunks)pkg/wstore/wstore_rtinfo.go(3 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-14T06:30:54.763Z
Learnt from: sawka
PR: wavetermdev/waveterm#2430
File: frontend/app/aipanel/aimessage.tsx:137-144
Timestamp: 2025-10-14T06:30:54.763Z
Learning: In `frontend/app/aipanel/aimessage.tsx`, the `AIToolUseGroup` component splits file operation tool calls into separate batches (`fileOpsNeedApproval` and `fileOpsNoApproval`) based on their approval state before passing them to `AIToolUseBatch`. This ensures each batch has homogeneous approval states, making group-level approval handling valid.
Applied to files:
frontend/app/aipanel/aitooluse.tsx
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
PR: wavetermdev/waveterm#2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Applied to files:
frontend/app/aipanel/aitooluse.tsx
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
PR: wavetermdev/waveterm#2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/store/global.ts
🧬 Code graph analysis (22)
frontend/app/aipanel/aitooluse.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(34-432)
pkg/wshrpc/wshrpctypes.go (1)
pkg/waveobj/waveobj.go (1)
ORef(30-34)
frontend/builder/builder-apppanel.tsx (3)
frontend/builder/tabs/builder-previewtab.tsx (1)
BuilderPreviewTab(16-16)frontend/builder/tabs/builder-filestab.tsx (1)
BuilderFilesTab(16-16)frontend/builder/tabs/builder-codetab.tsx (1)
BuilderCodeTab(16-16)
frontend/builder/builder-app.tsx (2)
frontend/app/store/keymodel.ts (1)
appHandleKeyDown(667-667)frontend/builder/builder-workspace.tsx (1)
BuilderWorkspace(118-118)
frontend/app/store/keymodel.ts (3)
frontend/app/store/focusManager.ts (1)
FocusManager(11-93)frontend/app/store/global.ts (4)
globalStore(834-834)atoms(811-811)getBlockComponentModel(822-822)getApi(821-821)frontend/layout/lib/layoutModelHooks.ts (1)
getLayoutModelForStaticTab(45-48)
pkg/waveappstore/waveappstore.go (2)
pkg/util/fileutil/fileutil.go (2)
EditSpec(258-262)ReplaceInFile(264-302)pkg/util/fileutil/readdir.go (3)
ReadDirResult(28-36)ReadDirRecursive(139-234)ReadDir(38-137)
frontend/builder/builder-workspace.tsx (4)
frontend/app/store/global.ts (1)
atoms(811-811)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/builder/builder-apppanel.tsx (1)
BuilderAppPanel(65-65)
emain/emain-builder.ts (5)
frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)emain/emain-wsh.ts (1)
ElectronWshClient(74-74)emain/emain-window.ts (3)
calculateWindowBounds(33-90)MinWindowWidth(30-30)MinWindowHeight(31-31)emain/emain-platform.ts (3)
unamePlatform(270-270)getElectronAppBasePath(260-260)isDevVite(268-268)emain/emain-events.ts (1)
globalEvents(30-30)
pkg/aiusechat/tools_builder.go (4)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(36-42)pkg/aiusechat/uctypes/usechat-types.go (3)
ToolDefinition(78-90)ApprovalNeedsApproval(129-129)ApprovalAutoApproved(133-133)pkg/waveappstore/waveappstore.go (3)
WriteAppFile(239-263)ReplaceInAppFile(287-303)ListAllAppFiles(336-351)pkg/util/fileutil/fileutil.go (1)
EditSpec(258-262)
pkg/util/fileutil/readdir.go (1)
pkg/util/utilfn/utilfn.go (1)
FormatRelativeTime(1114-1154)
frontend/layout/lib/layoutModel.ts (3)
frontend/app/store/focusManager.ts (1)
FocusManager(11-93)frontend/layout/lib/layoutTree.ts (2)
insertNodeAtIndex(297-320)magnifyNodeToggle(406-422)frontend/layout/lib/types.ts (2)
LayoutTreeInsertNodeAtIndexAction(165-172)LayoutTreeMagnifyNodeToggleAction(277-284)
emain/emain-ipc.ts (10)
emain/emain-window.ts (2)
focusedWaveWindow(96-96)getWaveWindowByWebContentsId(625-631)frontend/util/util.ts (1)
fireAndForget(434-434)emain/emain-platform.ts (2)
callWithOriginalXdgCurrentDesktopAsync(259-259)unamePlatform(270-270)frontend/util/endpoints.ts (1)
getWebServerEndpoint(10-10)emain/emain-tabview.ts (1)
getWaveTabViewByWebContentsId(31-33)emain/emain-wavesrv.ts (1)
getWaveVersion(28-30)emain/emain-util.ts (1)
handleCtrlShiftState(32-61)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)emain/emain-wsh.ts (1)
ElectronWshClient(74-74)emain/emain-builder.ts (1)
getBuilderWindowByWebContentsId(26-28)
pkg/wshrpc/wshserver/wshserver.go (2)
pkg/wstore/wstore_rtinfo.go (1)
DeleteRTInfo(115-120)pkg/waveobj/waveobj.go (1)
ORef(30-34)
pkg/wstore/wstore_rtinfo.go (2)
pkg/waveobj/waveobj.go (1)
ORef(30-34)pkg/waveobj/objrtinfo.go (1)
ObjRTInfo(6-24)
emain/emain-window.ts (1)
emain/emain-util.ts (1)
ensureBoundsAreVisible(205-216)
frontend/app/store/wshrpcutil.ts (6)
frontend/app/store/ws.ts (3)
WSControl(251-251)initGlobalWS(254-254)globalWS(253-253)pkg/wshutil/wshrouter.go (2)
WshRouter(51-59)DefaultRouter(85-85)frontend/app/store/wshrpcutil-base.ts (1)
setDefaultRouter(139-139)pkg/eventbus/eventbus.go (1)
WSEventType(22-26)frontend/util/endpoints.ts (1)
getWSServerEndpoint(12-12)frontend/app/store/tabrpcclient.ts (1)
TabClient(11-92)
frontend/wave.ts (7)
frontend/app/store/global.ts (6)
getApi(821-821)WOS(861-861)globalStore(834-834)atoms(811-811)initGlobal(835-835)loadConnStatus(838-838)frontend/app/store/wshrpcutil.ts (2)
initWshrpc(37-37)TabRpcClient(37-37)frontend/app/store/wshrouter.ts (2)
makeTabRouteId(156-156)makeBuilderRouteId(156-156)frontend/app/store/keymodel.ts (3)
globalRefocus(671-671)registerBuilderGlobalKeys(673-673)registerElectronReinjectKeyHandler(675-675)frontend/app/view/codeeditor/codeeditor.tsx (1)
loadMonaco(44-83)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/builder/builder-app.tsx (1)
BuilderApp(28-47)
emain/emain.ts (4)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
TEventProps(77-139)emain/emain-builder.ts (2)
getAllBuilderWindows(30-32)focusedBuilderWindow(20-20)emain/emain-window.ts (1)
getAllWaveWindows(645-647)emain/emain-ipc.ts (1)
initIpcHandlers(159-412)
frontend/app/aipanel/waveai-model.tsx (5)
pkg/waveobj/waveobj.go (1)
ORef(30-34)frontend/builder/store/builderFocusManager.ts (1)
BuilderFocusManager(9-34)frontend/app/store/focusManager.ts (1)
FocusManager(11-93)frontend/util/endpoints.ts (1)
getWebServerEndpoint(10-10)frontend/app/aipanel/aitypes.ts (1)
WaveUIMessage(24-24)
frontend/app/aipanel/aifeedbackbuttons.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(34-432)
pkg/web/ws.go (1)
pkg/eventbus/eventbus.go (1)
RegisterWSChannel(36-43)
emain/emain-menu.ts (3)
emain/emain-window.ts (2)
WaveBrowserWindow(134-615)focusedWaveWindow(96-96)emain/emain-platform.ts (1)
isDev(267-267)emain/emain-builder.ts (2)
createBuilderWindow(34-114)focusedBuilderWindow(20-20)
🪛 LanguageTool
aiprompts/tsunami-builder.md
[style] ~18-~18: In American English, abbreviations like “etc.” require a period.
Context: ...iles** - Static assets browser (images, etc) **Bottom Section - Build Panel (closa...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.18.1)
aiprompts/tsunami-builder.md
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
86-86: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
95-95: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
214-214: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (go)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (73)
pkg/waveobj/wtype.go (2)
43-43: LGTM!Correctly added to
ValidOTypesmap, enabling the new object type to be used inWaveObjUpdatestructures and pass validation checks.
32-33: OType_Builder constant declaration is correct; non-persistence is properly enforced.The "not persisted to DB" constraint is enforced by design: AllWaveObjTypes() at lines 298-305 excludes Builder and only returns the 6 types that are registered and persisted. Meanwhile, ValidOTypes includes OType_Builder, allowing it to be recognized as a valid object type in WaveObjUpdate structures. No code attempts to bypass this mechanism. The implementation pattern matches OType_Temp (also valid but not persisted).
pkg/util/fileutil/readdir.go (1)
139-152: LGTM!Path expansion, validation, and directory checking are correctly implemented and follow the same pattern as the non-recursive
ReadDirfunction.emain/emain-tabview.ts (1)
12-12: LGTM! Clean module reorganization.The import path update aligns with the platform abstraction refactoring across the Electron main process. The utilities are correctly used at lines 57, 78, and 81.
emain/emain-log.ts (1)
8-8: LGTM! Consistent with platform module refactoring.The import path update follows the same pattern as other emain files. The imported utilities are correctly used throughout the logging infrastructure.
emain/emain-wsh.ts (1)
9-9: LGTM! Platform abstraction refactoring applied consistently.The import source change maintains the same functionality while aligning with the new module structure.
emain/launchsettings.ts (1)
6-6: LGTM! Platform utility import updated correctly.The import path change is consistent with the broader emain-platform consolidation effort.
pkg/web/ws.go (1)
271-296: LGTM! Well-coordinated architectural shift from tab-based to route-based routing.The transition from
tabIdtorouteIdis complete and consistent:
- Query parameter extraction updated (line 271)
- Error message updated (line 273)
- Logging updated (line 290)
- Registration call updated (lines 291, 295-296)
This aligns with the broader builder window infrastructure and the matching changes in
pkg/eventbus/eventbus.go(RegisterWSChannel signature).emain/emain-wavesrv.ts (1)
19-20: LGTM! Import organization improved.The split between platform utilities (emain-platform) and other utilities (emain-util) improves module cohesion and follows the same refactoring pattern applied across the Electron main process.
pkg/aiusechat/usechat.go (1)
573-574: LGTM! Backward-compatible extension for builder context.The changes appropriately support both tab and builder contexts:
- Making
TabIdoptional withomitemptyallows builder-only requests- Adding
BuilderIdenables AI chat functionality in builder windows- Both fields being optional provides flexibility for different routing contexts
This aligns with the route-based architecture introduced across the PR.
pkg/eventbus/eventbus.go (3)
30-31: LGTM! Field rename aligns with route-based architecture.The
TabId→RouteIdrename in theWindowWatchDatastruct is consistent with the broader architectural shift to support route-based identifiers for both tab and builder windows.
36-42: LGTM! Public API updated consistently.The
RegisterWSChannelsignature change fromtabIdtorouteIdis properly reflected in:
- Parameter name (line 36)
- Field assignment (line 41)
- Caller in
pkg/web/ws.go(line 291)This maintains consistency across the WebSocket routing infrastructure.
51-61: LGTM! Filtering logic updated correctly.The comparison at line 56 now uses
RouteIdinstead ofTabId, maintaining the function's intent while supporting the new route-based model.emain/preload.ts (1)
54-54: LGTM! Clean IPC API additions.The new builder-specific IPC methods follow the established pattern (similar to
onWaveInit) and properly expose the builder window lifecycle events and commands.Also applies to: 62-62
pkg/wshutil/wshrouter.go (1)
33-33: LGTM! Consistent route prefix addition.The new builder route prefix and
MakeBuilderRouteIdhelper follow the established pattern used by other route types (tab, proc, conn, etc.).Also applies to: 81-83
frontend/builder/tabs/builder-previewtab.tsx (1)
6-16: LGTM! Appropriate scaffolding for preview tab.The component structure is clean and follows React best practices. As scaffolding for the builder UI, a simple placeholder is appropriate at this stage.
emain/emain-menu.ts (4)
28-44: LGTM! Correct window type handling.The reordering to check
BrowserWindowfirst (for builder windows) beforeWaveBrowserWindow(for main windows with tab views) properly handles both window types.
95-100: LGTM! Appropriate dev-only feature.The "New Tsunami App" menu item is properly gated behind
isDevand integrates cleanly with the builder window creation.
88-93: Verify the removal of the close accelerator.The accelerator for the "close" menu item has been set to an empty string. Please confirm this is intentional and won't impact user experience for closing windows via keyboard shortcuts.
194-198: LGTM! Context-aware menu labels and visibility.The dynamic menu behavior provides better UX:
- "Reload Window" vs "Reload Tab" clarifies the action scope
- Hiding the Workspace menu when builder is focused makes sense since workspaces don't apply to builder windows
Also applies to: 332-338
frontend/app/store/ws.ts (1)
40-40: LGTM! Consistent routing identifier rename.The systematic rename from
tabIdtorouteIdthroughout WSControl aligns with the broader shift to route-based identifiers. The changes are consistent across property declarations, parameters, and URL construction.Also applies to: 53-53, 59-59, 78-78, 234-234, 238-238
pkg/aiusechat/tools_builder.go (3)
15-15: LGTM! Appropriate approval gate for write operations.The write tool correctly requires approval before writing
app.go. The hardcoded filename aligns with the V1 scope (single-file apps pertsunami-builder.md).Also applies to: 39-80
82-167: LGTM! Proper edit tool implementation.The edit tool implements the str_replace pattern described in
tsunami-builder.mdwith:
- Array of edit specifications (old_str, new_str, desc)
- Approval requirement for safety
- Clear feedback with edit count
169-196: LGTM! Appropriate auto-approval for read-only operation.The list files tool correctly uses
ApprovalAutoApprovedsince it only reads data without making changes.frontend/app/store/global.ts (4)
36-44: LGTM! Flexible initialization for multiple window types.Making
tabIdoptional and addingbuilderIdproperly supports both tab-based and builder window contexts.
56-60: LGTM! Clean window type derivation.The
waveWindowTypeAtomcleanly derives the window type frombuilderIdpresence, providing a clear API for components to check context.Also applies to: 174-175
34-34: LGTM! Improved caching architecture.The refactoring to oref-based caching provides:
- Unified caching mechanism for all object types (blocks, connections, etc.)
- Better extensibility for new object types
- Consistent API via
getOrefMetaKeyAtomanduseOrefMetaKeyAtomAlso applies to: 285-303, 391-409
810-862: API migration from tab-specific to oref-based meta key access is complete.Verification confirms all references to the old
getTabMetaKeyAtom/useTabMetaKeyAtomAPI have been removed from the codebase. The newgetOrefMetaKeyAtom/useOrefMetaKeyAtomAPI is properly exported and actively used inworkspace-layout-model.tsandwaveai-model.tsx. No breaking references remain.tsconfig.json (1)
19-19: LGTM! Path alias properly configured.The new
@/builder/*path alias is consistent with existing path mappings and will simplify imports across the builder modules introduced in this PR.pkg/wshrpc/wshrpctypes.go (1)
863-865: LGTM! Delete flag properly added.The new
Deletefield enables explicit deletion of RTInfo entries through the SetRTInfo command. Theomitemptytag is appropriate for this optional flag.frontend/app/store/focusManager.ts (1)
11-33: LGTM! Clean singleton pattern refactor.The conversion from module-level export to class-based singleton with private constructor and
getInstance()provides better encapsulation while maintaining the same functionality. This pattern is consistent with the WaveAIModel singleton approach used elsewhere in the builder integration.frontend/builder/tabs/builder-codetab.tsx (1)
6-12: Scaffolding component approved.This is a placeholder component as part of the builder infrastructure scaffolding. The structure is clean and follows React best practices with
memoanddisplayName.frontend/app/aipanel/aifeedbackbuttons.tsx (1)
23-23: LGTM! Feedback routing centralized.The refactor to route feedback through
WaveAIModel.getInstance().handleAIFeedback()centralizes AI interaction logic and aligns with the architectural pattern established across this PR for builder/tab context handling.pkg/wshrpc/wshserver/wshserver.go (1)
170-173: LGTM! Delete logic properly implemented.The early-exit pattern for deletion is clean and straightforward. The implementation correctly calls
wstore.DeleteRTInfo()when theDeleteflag is set, aligning with the new field inCommandSetRTInfoData.pkg/waveobj/objrtinfo.go (1)
21-23: LGTM! RTInfo fields properly extended.The addition of
BuilderLayoutandWaveAIChatIdfields with appropriate JSON tags extends ObjRTInfo to support per-builder layout state and per-context chat session tracking. The use ofomitemptyis appropriate for these optional fields.frontend/app/aipanel/aipanelheader.tsx (1)
46-76: LGTM! Builder-aware UI conditional.Hiding the widget context toggle when in builder mode is a sensible UX decision, as the widget access concept may not apply to the builder context. The conditional rendering is clean and doesn't affect the toggle's functionality when visible.
frontend/app/store/wshrouter.ts (1)
26-28: LGTM!Clean addition of builder route ID helper that follows the existing pattern established by
makeTabRouteIdandmakeFeBlockRouteId.frontend/app/aipanel/aipanelmessages.tsx (1)
17-17: LGTM!The refactor to use
WaveAIModel.getPanelVisibleAtom()centralizes AI panel state management in a single model, improving maintainability.frontend/builder/tabs/builder-filestab.tsx (1)
1-16: LGTM!Clean placeholder component for the Files tab. The scaffolding follows React best practices with memoization and displayName set for debugging.
frontend/app/workspace/workspace-layout-model.ts (1)
95-103: LGTM!The migration from
getTabMetaKeyAtomtogetOrefMetaKeyAtomwith explicit ORef construction aligns with the broader oref-based storage architecture. The storage keys remain consistent, ensuring backward compatibility.frontend/types/gotypes.d.ts (2)
293-293: LGTM!The addition of the optional
deleteflag extends the RTInfo mutation capability cleanly without breaking existing usage.
724-725: LGTM!The new optional fields for builder layout and Wave AI chat ID extend the runtime info metadata without breaking existing contracts.
frontend/builder/builder-apppanel.tsx (1)
1-65: LGTM!Clean implementation of the builder app panel with a tabbed interface. The component follows React best practices with proper memoization, displayName, and straightforward state management. The conditional rendering and styling are well-structured.
frontend/app/aipanel/aitooluse.tsx (1)
8-8: LGTM!The refactor to centralize tool approval and keepalive logic through
WaveAIModelimproves maintainability by providing a single point of control for AI tool interactions. All call sites are updated consistently.Also applies to: 86-86, 96-96, 103-103, 148-148, 164-164, 169-169
frontend/app/store/wshrpcutil.ts (1)
13-28: LGTM!The refactor from
tabIdtorouteIdgeneralizes WebSocket RPC initialization to support both tab and builder contexts. The addition ofwpsReconnectHandlerensures proper reconnection behavior across all route types.frontend/layout/lib/layoutModel.ts (3)
4-4: FocusManager singleton import — OKSwitch to importing FocusManager and using its singleton is consistent with the new API.
596-598: Focus nudges on layout actions — OKCalling FocusManager.getInstance().requestNodeFocus() on focus/magnify/insert keeps focusType in sync with layout. No side‑effects (request* only sets the atom).
Also applies to: 602-604, 639-640, 643-644
1037-1038: Reactive focus check — OKReading get(FocusManager.getInstance().focusType) inside isFocused keeps node focus state reactive to global focusType.
frontend/builder/builder-app.tsx (2)
4-6: Import alias checkVerify "@/store/global" exists; elsewhere the project uses "@/app/store/global". If not aliased, change to "@/app/store/global" as in the diff above.
29-31: First‑render effect — OKOne‑time onFirstRender invocation is fine; intentional empty deps.
frontend/app/aipanel/aipanelinput.tsx (1)
24-25: Model‑scoped focus/panel wiring — looks good; confirm atom stabilityThe move to model.isWaveAIFocusedAtom / model.requestWaveAIFocus()/requestNodeFocus() and builder‑specific placeholder is sound.
Please confirm model.getPanelVisibleAtom() returns a stable, memoized Atom instance. If it allocates per call, Jotai subscriptions will churn. If needed, expose a pre‑created atom field instead (e.g., panelVisibleAtom).
Also applies to: 27-27, 60-61, 72-73, 133-133
frontend/types/custom.d.ts (1)
11-13: Builder mode surface area — OK; verify preload/main wiringNew atoms and IPC types look correct for builder windows.
Confirm:
- frontend/preload.ts exposes onBuilderInit and closeBuilderWindow matching these types.
- emain/emain-ipc.ts registers the "builder-init" channel and sends init opts.
- waveWindowType is set to "builder" during builder init.
Also applies to: 68-74, 115-116, 123-124
frontend/app/store/keymodel.ts (4)
163-171: genericClose uses FocusManager state — OKFocus decision branches on FocusManager.getInstance().getFocusType(); logic preserved.
210-232: Directional focus with FocusManager — OKLeft/right handling between WaveAI and nodes is clear and matches previous behavior.
286-289: Skip refocus in builder windows — good guardEarly‑return when waveWindowType === "builder" prevents tab refocus in builder.
413-427: Block key dispatch restricted to tab windows — good separationEnsures builder windows don’t call tab‑only block handlers.
emain/emain-window.ts (1)
30-32: Centralized bounds calc looks solidGood export of MinWindowWidth/MinWindowHeight and consolidation into calculateWindowBounds; parsing settings["window:dimensions"] and ensureBoundsAreVisible keeps windows on-screen. LGTM.
Also applies to: 33-90
frontend/app/aipanel/aipanel.tsx (2)
222-241: Transport + builder-aware UI wiring looks goodUsing model.getUseChatEndpointUrl and builder/tab id branching is clean. Conditional AIBuilderWelcomeMessage renders correctly. LGTM.
Also applies to: 475-483
222-241: No issues found—implementation is correctDefaultChatTransport constructor accepts an api parameter for the endpoint, and the current implementation properly supplies it via
model.getUseChatEndpointUrl(). The endpoint returns an absolute URL (http://localhost:port/api/post-chat-message) viagetWebServerEndpoint(), which is the correct format and is used consistently throughout the codebase. This works correctly in both development and packaged builds sincegetWebServerEndpoint()reads from an environment variable.emain/emain.ts (1)
14-14: Good integration of IPC init and builder window lifecycle
- initIpcHandlers() at startup is the right consolidation.
- before-quit hides both app and builder windows.
- windows-updated debounced via state tracking reduces churn. LGTM.
Also applies to: 240-261, 305-316, 342-343
emain/emain-builder.ts (1)
70-83: Init timing is covered; nice fallback via set-window-init-statusSending builder-init immediately and again on “ready” through savedInitOpts ensures delivery. LGTM.
pkg/waveappstore/waveappstore.go (6)
17-56: LGTM! Validation logic is sound.The namespace and app name validation patterns are well-defined, and the error messages accurately reflect the regex patterns. The ParseAppId function properly validates the format and handles empty components.
211-237: Excellent path validation security implementation.This function properly prevents directory traversal attacks through multiple defense layers:
- Rejects absolute paths
- Detects ".." sequences before resolution
- Resolves both paths to absolute form and verifies containment
- Handles the edge case where
fileNameis "." (line 232)The security approach is thorough and well-implemented.
239-334: LGTM! File operations are secure and well-implemented.All file operations properly:
- Validate app IDs
- Use
validateAndResolveFilePathfor security- Handle errors with context-rich messages
- Create necessary parent directories where needed
DeleteAppFilecorrectly usesos.Remove(notRemoveAll) to prevent accidental directory deletion.RenameAppFileproperly validates both source and destination paths.
353-396: LGTM! Listing logic is correct, with minor error handling consideration.The function properly:
- Handles missing waveapps directory gracefully
- Validates app IDs before including them
- Continues on individual namespace/app read errors (lines 377-379)
The silent error handling at line 378 (
continue) is acceptable for listing operations, though consider logging these errors for debugging purposes in the future.
397-452: LGTM! Editable apps logic correctly prioritizes local over draft.The function properly combines local and draft apps into a unified list. The logic at lines 444-448 correctly prioritizes local apps over draft versions when both exist for the same app name, which aligns with the expected workflow (drafts are working copies of local apps).
455-476: LGTM! Helper function is correctly implemented.The function properly:
- Validates the app ID is in the draft namespace
- Checks for the existence of a corresponding local version
- Returns
false, nilwhen the local version doesn't exist (not an error condition)frontend/app/aipanel/waveai-model.tsx (6)
44-45: LGTM! Constructor refactoring properly supports builder and tab contexts.The changes correctly:
- Make the constructor private with explicit
orefContextandinBuilderparameters- Initialize new atoms (
isWaveAIFocusedAtom,panelVisibleAtom) with context-aware logic- Handle builder-specific behavior (always-enabled widget access, always-visible panel)
- Delegate focus management to the appropriate manager (
BuilderFocusManagervsFocusManager)The scoped state management using
orefContextenables proper isolation between builder and tab instances.Also applies to: 57-58, 60-97
103-118: LGTM! Singleton initialization correctly determines context.The refactored
getInstanceproperly:
- Detects window type (builder vs tab) from global atoms
- Creates the appropriate
ORefcontext (builder/builderId vs tab/tabId)- Passes both
orefContextandinBuilderflag to the constructorThis enables the singleton pattern while supporting context-specific instances.
99-101: LGTM! Helper methods are straightforward and useful.
getPanelVisibleAtom()provides proper public access to the panel visibility atomgetUseChatEndpointUrl()correctly constructs the chat endpoint URL usinggetWebServerEndpoint()Also applies to: 124-126
180-183: LGTM! Methods correctly updated to use oref-based context.The updates properly:
- Use
SetRTInfoCommandwithorefContextfor RT data (line 180-183)- Skip panel visibility toggling for builder windows (line 221), since builders always show the panel
- Pass
orefContexttoSetMetaCommandfor model and widget access settingsAll RPC calls are now correctly scoped to the instance's context.
Also applies to: 220-227, 252-264
266-292: LGTM! Chat initialization properly refactored for oref-based context.The changes correctly:
- Return messages from
loadInitialChatinstead of setting them directly (cleaner separation)- Use
GetRTInfoCommandandSetRTInfoCommandwithorefContextfor chat ID management- Handle the cold-start case by generating a new chat ID when none exists (lines 271-277)
- Provide helpful type assertion comment (line 284) explaining RPC/FE type compatibility
- Update
uiLoadInitialChatto consume the returned messages (lines 363-364)Also applies to: 361-369
386-431: LGTM! New public methods are well-implemented.The additions properly:
- Record AI feedback telemetry with appropriate event structure (lines 386-397)
- Delegate focus management to the correct manager based on
inBuilderflag (lines 399-413)- Implement tool use workflows with keepalive and approval mechanisms (lines 415-431)
All methods correctly handle the distinction between builder and tab contexts.
| export function initIpcHandlers() { | ||
| electron.ipcMain.on("open-external", (event, url) => { | ||
| if (url && typeof url === "string") { | ||
| fireAndForget(() => | ||
| callWithOriginalXdgCurrentDesktopAsync(() => | ||
| electron.shell.openExternal(url).catch((err) => { | ||
| console.error(`Failed to open URL ${url}:`, err); | ||
| }) | ||
| ) | ||
| ); | ||
| } else { | ||
| console.error("Invalid URL received in open-external event:", url); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Restrict open-external to safe protocols
Validate and allowlist protocols (http, https, mailto) to avoid unexpected handlers.
Apply:
electron.ipcMain.on("open-external", (event, url) => {
- if (url && typeof url === "string") {
+ if (typeof url === "string") {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ console.error("Invalid URL received in open-external event:", url);
+ return;
+ }
+ const allowed = new Set(["http:", "https:", "mailto:"]);
+ if (!allowed.has(parsed.protocol)) {
+ console.error("Blocked URL with unsupported protocol:", parsed.protocol);
+ return;
+ }
fireAndForget(() =>
callWithOriginalXdgCurrentDesktopAsync(() =>
electron.shell.openExternal(url).catch((err) => {
console.error(`Failed to open URL ${url}:`, err);
})
)
);
} else {
console.error("Invalid URL received in open-external event:", url);
}
});🤖 Prompt for AI Agents
In emain/emain-ipc.ts around lines 159 to 173, the open-external handler
currently forwards any string URL to shell.openExternal; restrict it by parsing
the URL (using the URL constructor inside a try/catch), normalize and extract
the protocol, and only allowlist protocols "http:", "https:", and "mailto:"
(reject others and log an error). If parsing fails or the protocol is not
allowed, do not call shell.openExternal and instead log the invalid
URL/protocol. Keep the existing
fireAndForget/callWithOriginalXdgCurrentDesktopAsync flow when the URL is valid.
| function registerBuilderGlobalKeys() { | ||
| globalKeyMap.set("Cmd:w", () => { | ||
| getApi().closeBuilderWindow(); | ||
| return true; | ||
| }); | ||
| const allKeys = Array.from(globalKeyMap.keys()); | ||
| getApi().registerGlobalWebviewKeys(allKeys); | ||
| } | ||
|
|
There was a problem hiding this comment.
Builder key registration exists; ensure it’s called
registerBuilderGlobalKeys maps Cmd+w and registers keys; make sure builder windows call this once (see suggested diff in BuilderApp).
🤖 Prompt for AI Agents
In frontend/app/store/keymodel.ts around lines 652 to 660,
registerBuilderGlobalKeys is defined but not invoked; ensure builder windows
call this once by importing and invoking registerBuilderGlobalKeys from the
BuilderApp initialization/mount code (e.g., when the builder webview/component
mounts). Make the call idempotent (guard with a boolean or move registration
behind a module-level flag) so repeated mounts don't double-register, and ensure
you pass the correct context so getApi().registerGlobalWebviewKeys runs for the
builder window exactly once.
| const BuilderKeyHandlers = () => { | ||
| useEffect(() => { | ||
| const staticKeyDownHandler = keyutil.keydownWrapper(appHandleKeyDown); | ||
| document.addEventListener("keydown", staticKeyDownHandler); | ||
|
|
||
| return () => { | ||
| document.removeEventListener("keydown", staticKeyDownHandler); | ||
| }; | ||
| }, []); | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
Register builder keybindings on mount
Without registering, Cmd+w won’t be mapped and webview keys won’t be forwarded. Call registerBuilderGlobalKeys() once on mount.
Suggested diff:
import { BuilderWorkspace } from "@/builder/builder-workspace";
-import { globalStore } from "@/store/global";
+import { globalStore } from "@/app/store/global";
import { appHandleKeyDown } from "@/store/keymodel";
+import { registerBuilderGlobalKeys } from "@/app/store/keymodel";
...
const BuilderKeyHandlers = () => {
useEffect(() => {
+ registerBuilderGlobalKeys();
const staticKeyDownHandler = keyutil.keydownWrapper(appHandleKeyDown);
document.addEventListener("keydown", staticKeyDownHandler);
return () => {
document.removeEventListener("keydown", staticKeyDownHandler);
};
}, []);
return null;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/builder/builder-app.tsx around lines 16 to 26, the
BuilderKeyHandlers component currently only adds a document keydown listener but
never registers the builder global keybindings; to fix, call
registerBuilderGlobalKeys() once inside the useEffect when the component mounts
(before or after adding the keydown listener) so Cmd+W and webview key
forwarding are enabled; keep existing cleanup for the keydown listener and do
not re-register on updates by leaving the dependency array empty.
| func copyDir(src, dst string) error { | ||
| if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("failed to remove existing directory: %w", err) | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { | ||
| return fmt.Errorf("failed to create parent directory: %w", err) | ||
| } | ||
|
|
||
| return filepath.Walk(src, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| relPath, err := filepath.Rel(src, path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| dstPath := filepath.Join(dst, relPath) | ||
|
|
||
| if info.IsDir() { | ||
| return os.MkdirAll(dstPath, info.Mode()) | ||
| } | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(dstPath, data, info.Mode()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider atomic directory operations to prevent data loss.
The function removes the destination directory before copying (line 68), which could result in data loss if the copy operation fails partway through. Consider using a temporary directory with atomic rename:
- Copy to a temporary directory (e.g.,
dst + ".tmp") - On success, remove the old
dstand rename the temp directory - On failure, clean up the temp directory without touching
dst
This ensures that either the operation fully succeeds or the original destination remains intact.
func copyDir(src, dst string) error {
- if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) {
- return fmt.Errorf("failed to remove existing directory: %w", err)
- }
- if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
+ tmpDst := dst + ".tmp"
+ if err := os.RemoveAll(tmpDst); err != nil && !os.IsNotExist(err) {
+ return fmt.Errorf("failed to remove temporary directory: %w", err)
+ }
+ if err := os.MkdirAll(filepath.Dir(tmpDst), 0755); err != nil {
return fmt.Errorf("failed to create parent directory: %w", err)
}
- return filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
+ err := filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
relPath, err := filepath.Rel(src, path)
if err != nil {
return err
}
- dstPath := filepath.Join(dst, relPath)
+ dstPath := filepath.Join(tmpDst, relPath)
if info.IsDir() {
return os.MkdirAll(dstPath, info.Mode())
}
data, err := os.ReadFile(path)
if err != nil {
return err
}
return os.WriteFile(dstPath, data, info.Mode())
})
+ if err != nil {
+ os.RemoveAll(tmpDst)
+ return err
+ }
+
+ if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) {
+ os.RemoveAll(tmpDst)
+ return fmt.Errorf("failed to remove existing directory: %w", err)
+ }
+
+ if err := os.Rename(tmpDst, dst); err != nil {
+ os.RemoveAll(tmpDst)
+ return fmt.Errorf("failed to rename temporary directory: %w", err)
+ }
+
+ return nil
}📝 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 copyDir(src, dst string) error { | |
| if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) { | |
| return fmt.Errorf("failed to remove existing directory: %w", err) | |
| } | |
| if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { | |
| return fmt.Errorf("failed to create parent directory: %w", err) | |
| } | |
| return filepath.Walk(src, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| relPath, err := filepath.Rel(src, path) | |
| if err != nil { | |
| return err | |
| } | |
| dstPath := filepath.Join(dst, relPath) | |
| if info.IsDir() { | |
| return os.MkdirAll(dstPath, info.Mode()) | |
| } | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(dstPath, data, info.Mode()) | |
| }) | |
| } | |
| func copyDir(src, dst string) error { | |
| tmpDst := dst + ".tmp" | |
| if err := os.RemoveAll(tmpDst); err != nil && !os.IsNotExist(err) { | |
| return fmt.Errorf("failed to remove temporary directory: %w", err) | |
| } | |
| if err := os.MkdirAll(filepath.Dir(tmpDst), 0755); err != nil { | |
| return fmt.Errorf("failed to create parent directory: %w", err) | |
| } | |
| err := filepath.Walk(src, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| relPath, err := filepath.Rel(src, path) | |
| if err != nil { | |
| return err | |
| } | |
| dstPath := filepath.Join(tmpDst, relPath) | |
| if info.IsDir() { | |
| return os.MkdirAll(dstPath, info.Mode()) | |
| } | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(dstPath, data, info.Mode()) | |
| }) | |
| if err != nil { | |
| os.RemoveAll(tmpDst) | |
| return err | |
| } | |
| if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) { | |
| os.RemoveAll(tmpDst) | |
| return fmt.Errorf("failed to remove existing directory: %w", err) | |
| } | |
| if err := os.Rename(tmpDst, dst); err != nil { | |
| os.RemoveAll(tmpDst) | |
| return fmt.Errorf("failed to rename temporary directory: %w", err) | |
| } | |
| return nil | |
| } |
| } else if fieldValue.Kind() == reflect.Map { | ||
| // Handle map[string]float64 fields | ||
| if fieldValue.Type().Key().Kind() == reflect.String && fieldValue.Type().Elem().Kind() == reflect.Float64 { | ||
| if inputMap, ok := value.(map[string]any); ok { | ||
| outputMap := make(map[string]float64) | ||
| for k, v := range inputMap { | ||
| if floatVal, ok := v.(float64); ok { | ||
| outputMap[k] = floatVal | ||
| } | ||
| } | ||
| fieldValue.Set(reflect.ValueOf(outputMap)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Broaden map assignment: accept map[string]float64 and numeric values
Current code only handles map[string]any with float64 values. Legit inputs like map[string]float64 are ignored.
Apply this diff:
- } else if fieldValue.Kind() == reflect.Map {
- // Handle map[string]float64 fields
- if fieldValue.Type().Key().Kind() == reflect.String && fieldValue.Type().Elem().Kind() == reflect.Float64 {
- if inputMap, ok := value.(map[string]any); ok {
- outputMap := make(map[string]float64)
- for k, v := range inputMap {
- if floatVal, ok := v.(float64); ok {
- outputMap[k] = floatVal
- }
- }
- fieldValue.Set(reflect.ValueOf(outputMap))
- }
- }
- } else if fieldValue.Kind() == reflect.Interface {
+ } else if fieldValue.Kind() == reflect.Map {
+ // Handle map[string]float64 fields
+ if fieldValue.Type().Key().Kind() == reflect.String && fieldValue.Type().Elem().Kind() == reflect.Float64 {
+ switch input := value.(type) {
+ case map[string]float64:
+ out := make(map[string]float64, len(input))
+ for k, v := range input {
+ out[k] = v
+ }
+ fieldValue.Set(reflect.ValueOf(out))
+ case map[string]any:
+ out := make(map[string]float64, len(input))
+ for k, vv := range input {
+ switch t := vv.(type) {
+ case float64:
+ out[k] = t
+ case int:
+ out[k] = float64(t)
+ case int64:
+ out[k] = float64(t)
+ case float32:
+ out[k] = float64(t)
+ }
+ }
+ fieldValue.Set(reflect.ValueOf(out))
+ }
+ }
+ } else if fieldValue.Kind() == reflect.Interface {
// Handle any/interface{} fields
fieldValue.Set(reflect.ValueOf(value))
}Optionally extend int handling to accept more numeric kinds and json.Number.
| // GetRTInfo returns the ObjRTInfo for the given ORef, or nil if not found | ||
| func GetRTInfo(oref waveobj.ORef) *waveobj.ObjRTInfo { | ||
| blockRTInfoMutex.RLock() | ||
| defer blockRTInfoMutex.RUnlock() | ||
| rtInfoMutex.RLock() | ||
| defer rtInfoMutex.RUnlock() | ||
|
|
||
| if rtInfo, exists := blockRTInfoStore[oref]; exists { | ||
| if rtInfo, exists := rtInfoStore[oref]; exists { | ||
| // Return a copy to avoid external modification | ||
| copy := *rtInfo | ||
| return © | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Deep-copy map fields in GetRTInfo to prevent external mutation and data races
Shallow copying the struct aliases map fields (e.g., BuilderLayout). Callers can mutate internal state without locks.
Apply this diff:
- if rtInfo, exists := rtInfoStore[oref]; exists {
- // Return a copy to avoid external modification
- copy := *rtInfo
- return ©
- }
+ if rtInfo, exists := rtInfoStore[oref]; exists {
+ // Return a defensive copy (deep copy map fields)
+ copy := *rtInfo
+ if rtInfo.BuilderLayout != nil {
+ m := make(map[string]float64, len(rtInfo.BuilderLayout))
+ for k, v := range rtInfo.BuilderLayout {
+ m[k] = v
+ }
+ copy.BuilderLayout = m
+ }
+ return ©
+ }📝 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.
| // GetRTInfo returns the ObjRTInfo for the given ORef, or nil if not found | |
| func GetRTInfo(oref waveobj.ORef) *waveobj.ObjRTInfo { | |
| blockRTInfoMutex.RLock() | |
| defer blockRTInfoMutex.RUnlock() | |
| rtInfoMutex.RLock() | |
| defer rtInfoMutex.RUnlock() | |
| if rtInfo, exists := blockRTInfoStore[oref]; exists { | |
| if rtInfo, exists := rtInfoStore[oref]; exists { | |
| // Return a copy to avoid external modification | |
| copy := *rtInfo | |
| return © | |
| } | |
| return nil | |
| } | |
| // GetRTInfo returns the ObjRTInfo for the given ORef, or nil if not found | |
| func GetRTInfo(oref waveobj.ORef) *waveobj.ObjRTInfo { | |
| rtInfoMutex.RLock() | |
| defer rtInfoMutex.RUnlock() | |
| if rtInfo, exists := rtInfoStore[oref]; exists { | |
| // Return a defensive copy (deep copy map fields) | |
| copy := *rtInfo | |
| if rtInfo.BuilderLayout != nil { | |
| m := make(map[string]float64, len(rtInfo.BuilderLayout)) | |
| for k, v := range rtInfo.BuilderLayout { | |
| m[k] = v | |
| } | |
| copy.BuilderLayout = m | |
| } | |
| return © | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In pkg/wstore/wstore_rtinfo.go around lines 101 to 112, the function returns a
shallow copy of ObjRTInfo so internal map/slice fields (e.g., BuilderLayout and
any other maps or slices on ObjRTInfo) can still be mutated by callers; update
GetRTInfo to construct and return a deep copy: allocate a new ObjRTInfo, copy
primitive fields, and for each map/slice field create new instances and copy
entries/elements (handle nils), then return a pointer to that deep-copied struct
while keeping the RLock/RUnlock as is.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
pkg/util/fileutil/fileutil.go (1)
301-303: Atomic write issue from previous review remains unaddressed.Direct
os.WriteFilecan leave a truncated or corrupted file on crash or interruption. The previous review comment provided a completewriteFileAtomichelper using temp file + fsync + rename.As suggested in the prior review, apply the atomic write pattern:
- if err := os.WriteFile(filePath, []byte(modifiedContents), fileInfo.Mode()); err != nil { - return fmt.Errorf("failed to write file: %w", err) - } + if err := writeFileAtomic(filePath, []byte(modifiedContents), fileInfo.Mode()); err != nil { + return fmt.Errorf("failed to write file atomically: %w", err) + }Add the helper function shown in the previous review (creates temp in same dir, writes, fsyncs, closes, removes original on Windows if needed, atomically renames).
pkg/waveappstore/waveappstore.go (1)
67-95: Atomic directory operations issue from previous review remains unaddressed.The function removes the destination directory before copying (line 68), risking data loss if the copy fails partway through. The previous review comment recommended using a temporary directory with atomic rename to ensure either full success or leaving the original destination intact.
As suggested in the prior review, use the atomic pattern with a temporary directory:
func copyDir(src, dst string) error { - if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove existing directory: %w", err) - } - if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { + tmpDst := dst + ".tmp" + if err := os.RemoveAll(tmpDst); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove temporary directory: %w", err) + } + if err := os.MkdirAll(filepath.Dir(tmpDst), 0755); err != nil { return fmt.Errorf("failed to create parent directory: %w", err) } - return filepath.Walk(src, func(path string, info os.FileInfo, err error) error { + err := filepath.Walk(src, func(path string, info os.FileInfo, err error) error { if err != nil { return err } relPath, err := filepath.Rel(src, path) if err != nil { return err } - dstPath := filepath.Join(dst, relPath) + dstPath := filepath.Join(tmpDst, relPath) if info.IsDir() { return os.MkdirAll(dstPath, info.Mode()) } data, err := os.ReadFile(path) if err != nil { return err } return os.WriteFile(dstPath, data, info.Mode()) }) + if err != nil { + os.RemoveAll(tmpDst) + return err + } + + if err := os.RemoveAll(dst); err != nil && !os.IsNotExist(err) { + os.RemoveAll(tmpDst) + return fmt.Errorf("failed to remove existing directory: %w", err) + } + + if err := os.Rename(tmpDst, dst); err != nil { + os.RemoveAll(tmpDst) + return fmt.Errorf("failed to rename temporary directory: %w", err) + } + + return nil }emain/emain-ipc.ts (2)
115-168: Add SVG to filters and guard missing filePath.Include "svg" in the save dialog filters and handle undefined file.filePath before writing.
const mimeToExtension: { [key: string]: string } = { "image/png": "png", "image/jpeg": "jpg", "image/gif": "gif", "image/webp": "webp", "image/bmp": "bmp", "image/tiff": "tiff", "image/heic": "heic", "image/svg+xml": "svg", }; @@ electron.dialog .showSaveDialog(ww, { title: "Save Image", defaultPath: defaultFileName, - filters: [{ name: "Images", extensions: ["png", "jpg", "jpeg", "gif", "webp", "bmp", "tiff", "heic"] }], + filters: [{ name: "Images", extensions: ["png", "jpg", "jpeg", "gif", "webp", "bmp", "tiff", "heic", "svg"] }], }) .then((file) => { - if (file.canceled) { + if (file.canceled || !file.filePath) { return; }
170-183: Restrict open-external to safe protocols (http/https/mailto).Parse and allowlist protocols to avoid unexpected handlers.
- electron.ipcMain.on("open-external", (event, url) => { - if (url && typeof url === "string") { - fireAndForget(() => - callWithOriginalXdgCurrentDesktopAsync(() => - electron.shell.openExternal(url).catch((err) => { - console.error(`Failed to open URL ${url}:`, err); - }) - ) - ); - } else { - console.error("Invalid URL received in open-external event:", url); - } - }); + electron.ipcMain.on("open-external", (event, url) => { + if (typeof url !== "string") { + console.error("Invalid URL received in open-external event:", url); + return; + } + let parsed: URL; + try { + parsed = new URL(url); + } catch { + console.error("Invalid URL received in open-external event:", url); + return; + } + const allowed = new Set(["http:", "https:", "mailto:"]); + if (!allowed.has(parsed.protocol)) { + console.error("Blocked URL with unsupported protocol:", parsed.protocol); + return; + } + fireAndForget(() => + callWithOriginalXdgCurrentDesktopAsync(() => + electron.shell.openExternal(url).catch((err) => { + console.error(`Failed to open URL ${url}:`, err); + }) + ) + ); + });
🧹 Nitpick comments (3)
frontend/app/store/global.ts (2)
56-61: Type the builderId atom as possibly undefined.initOpts.builderId is optional; coercing to PrimitiveAtom is unsafe. Type it accurately.
- const builderIdAtom = atom(initOpts.builderId) as PrimitiveAtom<string>; + const builderIdAtom: PrimitiveAtom<string | undefined> = atom(initOpts.builderId);
34-35: Optional: consider lifecycle cleanup for orefAtomCache.Long-lived Map keyed by strings can grow. Consider pruning on waveobj:delete or scoping by window.
emain/emain-ipc.ts (1)
85-106: Abort response cleanly on non‑2xx.After detecting non‑2xx, destroy the response to free resources.
- if (statusCode < 200 || statusCode >= 300) { - readable.destroy(); - request.abort(); - reject(new Error(`HTTP request failed with status ${statusCode}: ${response.statusMessage || ""}`)); - return; - } + if (statusCode == null || statusCode < 200 || statusCode >= 300) { + response.destroy(); // ensure socket cleanup + readable.destroy(); + request.abort(); + reject(new Error(`HTTP ${statusCode ?? "?"} fetching ${url}`)); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
emain/emain-ipc.ts(1 hunks)emain/emain-menu.ts(5 hunks)frontend/app/aipanel/aipanel.tsx(7 hunks)frontend/app/store/global.ts(8 hunks)frontend/util/util.ts(2 hunks)pkg/util/fileutil/fileutil.go(2 hunks)pkg/waveappstore/waveappstore.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emain/emain-menu.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
PR: wavetermdev/waveterm#2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/store/global.ts
🧬 Code graph analysis (2)
emain/emain-ipc.ts (10)
frontend/util/util.ts (2)
parseDataUrl(490-490)fireAndForget(478-478)emain/emain-window.ts (2)
focusedWaveWindow(96-96)getWaveWindowByWebContentsId(625-631)emain/emain-platform.ts (2)
callWithOriginalXdgCurrentDesktopAsync(259-259)unamePlatform(270-270)frontend/util/endpoints.ts (1)
getWebServerEndpoint(10-10)emain/emain-tabview.ts (1)
getWaveTabViewByWebContentsId(31-33)emain/emain-wavesrv.ts (1)
getWaveVersion(28-30)emain/emain-util.ts (1)
handleCtrlShiftState(32-61)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)emain/emain-wsh.ts (1)
ElectronWshClient(74-74)emain/emain-builder.ts (1)
getBuilderWindowByWebContentsId(26-28)
pkg/waveappstore/waveappstore.go (2)
pkg/util/fileutil/fileutil.go (2)
EditSpec(258-262)ReplaceInFile(264-306)pkg/util/fileutil/readdir.go (3)
ReadDirResult(28-36)ReadDirRecursive(139-234)ReadDir(38-137)
🪛 Biome (2.1.2)
frontend/app/store/global.ts
[error] 355-355: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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 (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (24)
pkg/util/fileutil/fileutil.go (1)
270-272: Regular file guard successfully implemented.The guard that rejects non-regular files addresses the previous review comment. This ensures directories, devices, and special files are not processed.
pkg/waveappstore/waveappstore.go (5)
126-126: Return value corrected: now returnslocalAppId.The function now correctly returns the published local app identifier instead of the draft identifier. This addresses the previous review feedback and provides the correct reference for callers.
182-187: Error handling clarified with correctly placed comment.The comment "draft already exists, don't overwrite" is now correctly positioned after the
os.Statcheck that actually detects an existing draft. This addresses the previous review feedback about the misleading error handling.
213-239: Path traversal protection is well implemented.The validation correctly:
- Rejects absolute paths
- Cleans the path and checks for ".." traversal attempts
- Resolves to absolute paths for comparison
- Verifies the resolved path stays within the app directory boundary
This provides strong protection against directory traversal attacks.
307-336: File rename operation is correctly implemented.The function properly:
- Validates both source and destination paths
- Creates the destination parent directory if needed
- Uses
os.Renamefor atomic rename on the same filesystem
446-450: Local apps correctly prioritized over draft apps.The logic correctly returns local app IDs when both local and draft versions exist for the same app name, which aligns with the expected precedence.
frontend/app/aipanel/aipanel.tsx (7)
165-181: Verify the builder welcome message content is complete.The
AIBuilderWelcomeMessageis significantly more minimal thanAIWelcomeMessage(which includes keyboard shortcuts, drag-drop instructions, widget context explanations, and a Discord link). Is this sparse content intentional for the builder context, or is this placeholder content pending final copy?
218-220: LGTM: Model-driven focus and visibility state.The refactor to use
model.isWaveAIFocusedAtomandmodel.getPanelVisibleAtom()centralizes state management in the WaveAIModel, improving consistency across builder and tab contexts.
279-279: LGTM: Clearer method naming.Renaming
uiLoadChattouiLoadInitialChatbetter conveys this is a one-time initialization during component mount.
383-389: LGTM: Centralized focus management.The refactor to use
model.requestWaveAIFocus()instead of directfocusManagercalls properly centralizes focus handling through the WaveAIModel, ensuring consistent behavior across builder and tab contexts.Also applies to: 401-401
452-462: LGTM: Context-appropriate styling.The conditional styling based on
model.inBuilderappropriately adjusts the panel appearance for different window contexts—removing right-side border radius and top margin in builder mode suggests flush integration with surrounding builder UI, while preserving rounded corners and spacing in tab mode.
483-483: LGTM: Context-aware welcome message.The conditional rendering correctly displays
AIBuilderWelcomeMessagein builder contexts andAIWelcomeMessagein tab contexts based onmodel.inBuilder.
224-238: Improve type safety for request body construction.The type annotations for
builderIdandwaveWindowTypeare misleading. ThebuilderIdatom is initialized from an optionalbuilderId?: string, yet it's typed asPrimitiveAtom<string>. ThewaveWindowTypederives its value by checkingbuilderId != null ? "builder" : "tab", so the logic ensures builderId is defined when windowType is "builder", but the type system doesn't reflect this guarantee.Replace the
anytype (line 228) with a proper interface to catch mismatches at compile time:- const body: any = { + const body = { msg, chatid: globalStore.get(model.chatId), widgetaccess: globalStore.get(model.widgetAccessAtom), }; + if (windowType === "builder") { + body.builderid = globalStore.get(atoms.builderId); + } else { + body.tabid = globalStore.get(atoms.staticTabId); + } + return { body };Consider defining a discriminated union type to enforce exactly one of
builderidortabidis present, improving type safety for the backend contract.frontend/app/store/global.ts (7)
285-299: LGTM: oref-based meta accessor generalizes tab/block lookups.The abstraction aligns with oref cache strategy and avoids duplicate caches.
301-303: LGTM: hook wrapper for oref meta.Simple, consistent with getOrefMetaKeyAtom.
395-397: LGTM: block cache delegates to oref cache.Good consolidation.
399-403: LGTM: conn cache wraps oref cache.Comment clarifies non-real oref usage; acceptable.
405-413: LGTM: centralized oref atom cache.Straightforward and efficient.
174-176: LGTM: exposing builder/window atoms.Makes window type derivation available to FE; matches new builder flows.
831-832: LGTM: new oref meta exports.Exports align with introduced APIs.
Also applies to: 861-862
emain/emain-ipc.ts (4)
63-75: Data URL handling depends on util.parseDataUrl; ensure Node-safe decode (no atob) and comma-preserving payload.Current main code is fine if parseDataUrl is robust. Please apply the util.ts fixes suggested in that file so Electron main never hits atob and supports commas/non-base64 correctly. Otherwise this path will throw in main.
Would you confirm parseDataUrl changes get bundled into the main process build?
Also applies to: 85-91
303-327: LGTM: WCO overlay sampling logic.Good use of capturePage + FastAverageColor; transparent on non-Linux, colorized on Linux.
340-351: LGTM: clear-webview-storage handler.Properly guards session and returns success.
404-422: LGTM: builder RTInfo cleanup on close.Deletes RTInfo before destroying window; good hygiene.
| function useOverrideConfigAtom<T extends keyof SettingsType>(blockId: string | null, key: T): SettingsType[T] { | ||
| if (blockId == null) { | ||
| return useAtomValue(getSettingsKeyAtom(key)); | ||
| } | ||
| return useAtomValue(getOverrideConfigAtom(blockId, key)); | ||
| } |
There was a problem hiding this comment.
Unconditional hook call to satisfy Rules of Hooks and Biome.
Call useAtomValue once, outside condition.
-function useOverrideConfigAtom<T extends keyof SettingsType>(blockId: string | null, key: T): SettingsType[T] {
- if (blockId == null) {
- return useAtomValue(getSettingsKeyAtom(key));
- }
- return useAtomValue(getOverrideConfigAtom(blockId, key));
-}
+function useOverrideConfigAtom<T extends keyof SettingsType>(blockId: string | null, key: T): SettingsType[T] {
+ const a = blockId == null ? getSettingsKeyAtom(key) : getOverrideConfigAtom(blockId, key);
+ return useAtomValue(a);
+}As per static analysis hint (Biome lint/correctness/useHookAtTopLevel).
📝 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.
| function useOverrideConfigAtom<T extends keyof SettingsType>(blockId: string | null, key: T): SettingsType[T] { | |
| if (blockId == null) { | |
| return useAtomValue(getSettingsKeyAtom(key)); | |
| } | |
| return useAtomValue(getOverrideConfigAtom(blockId, key)); | |
| } | |
| function useOverrideConfigAtom<T extends keyof SettingsType>(blockId: string | null, key: T): SettingsType[T] { | |
| const a = blockId == null ? getSettingsKeyAtom(key) : getOverrideConfigAtom(blockId, key); | |
| return useAtomValue(a); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 355-355: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 357-357: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In frontend/app/store/global.ts around lines 353 to 358, the code conditionally
calls the hook useAtomValue which violates the Rules of Hooks and Biome lint;
fix it by selecting the correct atom first (e.g., const atom = blockId == null ?
getSettingsKeyAtom(key) : getOverrideConfigAtom(blockId, key)) and then call
useAtomValue(atom) exactly once, ensuring the atom variable is typed
appropriately so the return type remains SettingsType[T].
| if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil { | ||
| return fmt.Errorf("failed to create directory: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(filePath, contents, 0644); err != nil { | ||
| return fmt.Errorf("failed to write file: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use atomic writes to prevent file corruption.
Direct os.WriteFile at line 260 can leave a corrupted file on crash or interruption. Apply the same atomic write pattern recommended for fileutil.ReplaceInFile (temp file + fsync + rename) to ensure write safety.
Consider creating a shared writeFileAtomic helper in fileutil and using it here:
- if err := os.WriteFile(filePath, contents, 0644); err != nil {
+ if err := fileutil.WriteFileAtomic(filePath, contents, 0644); err != nil {
return fmt.Errorf("failed to write file: %w", err)
}The writeFileAtomic implementation was detailed in the previous review of pkg/util/fileutil/fileutil.go.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/waveappstore/waveappstore.go around lines 256 to 262, replace the direct
os.WriteFile call with an atomic write using a shared helper (e.g.,
fileutil.WriteFileAtomic): keep the MkdirAll, then call the helper which should
create a temp file in the same directory, write contents, fsync the file, close
it, and then rename it into place (preserving permissions), and return any
error; if the helper doesn't exist, add fileutil.WriteFileAtomic that implements
temp file + fsync + rename semantics (create temp with same dir, write data,
call file.Sync(), close, os.Chmod if needed, then os.Rename) and use that helper
here instead of os.WriteFile to prevent corruption on crash/interruption.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/builder/builder-workspace.tsx (1)
24-24: Fix state nullability for layout.Initial state is
nullbut the type excludesnull; this will fail under strict TypeScript.Apply this diff:
- const [layout, setLayout] = useState<Record<string, number>>(null); + const [layout, setLayout] = useState<Record<string, number> | null>(null);
🧹 Nitpick comments (2)
frontend/builder/tabs/builder-codetab.tsx (1)
17-19: Consider adding accessibility attributes to the invisible input.The invisible input pattern for focus management is valid, but it should include accessibility attributes to prevent keyboard navigation and screen reader confusion.
Apply this diff to improve accessibility:
- <div key="focuselem" className="h-0 w-0"> - <input type="text" value="" ref={focusElemRef} className="h-0 w-0 opacity-0 pointer-events-none" onChange={() => {}} /> - </div> + <div key="focuselem" className="h-0 w-0"> + <input type="text" value="" ref={focusElemRef} className="h-0 w-0 opacity-0 pointer-events-none" onChange={() => {}} tabIndex={-1} aria-hidden="true" /> + </div>frontend/builder/tabs/builder-filestab.tsx (1)
18-24: Simplify the hidden input for focus management.The controlled input with
value=""and an emptyonChangehandler can be simplified. Since this input is only used for focus management and not for user input, consider making it uncontrolled or adding thereadOnlyattribute.Apply this diff to use a read-only input:
<div key="focuselem" className="h-0 w-0"> <input type="text" - value="" ref={focusElemRef} className="h-0 w-0 opacity-0 pointer-events-none" - onChange={() => {}} + readOnly /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/builder/builder-apppanel.tsx(1 hunks)frontend/builder/builder-workspace.tsx(1 hunks)frontend/builder/tabs/builder-codetab.tsx(1 hunks)frontend/builder/tabs/builder-filestab.tsx(1 hunks)frontend/builder/tabs/builder-previewtab.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/builder/tabs/builder-previewtab.tsx
- frontend/builder/builder-apppanel.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/builder/tabs/builder-filestab.tsx (1)
frontend/builder/store/builderFocusManager.ts (1)
BuilderFocusManager(9-34)
frontend/builder/tabs/builder-codetab.tsx (1)
frontend/builder/store/builderFocusManager.ts (1)
BuilderFocusManager(9-34)
frontend/builder/builder-workspace.tsx (5)
frontend/app/store/global.ts (1)
atoms(814-814)frontend/builder/store/builderFocusManager.ts (1)
BuilderFocusManager(9-34)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/builder/builder-apppanel.tsx (1)
BuilderAppPanel(88-88)
⏰ 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: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
frontend/builder/tabs/builder-codetab.tsx (1)
7-13: LGTM! Clean focus management integration.The component setup and click handler are well-implemented. The use of
memofor optimization, proper ref typing, and safe optional chaining on the focus call are all good practices. The integration withBuilderFocusManagercorrectly signals app focus when the tab is clicked.frontend/builder/builder-workspace.tsx (3)
73-89: Ensure layout handlers are null-safe.The layout change handlers spread
layoutat lines 75 and 84. While the early return at line 91 prevents rendering whenlayoutisnull, these handlers are created during render even whenlayoutisnull. If the component state changes such thatlayoutbecomesnullagain, subsequent invocations would fail.Consider adding null checks within the handlers:
const handleHorizontalLayout = useCallback( (sizes: number[]) => { + if (!layout) return; const newLayout = { ...layout, chat: sizes[0] }; setLayout(newLayout); saveLayout(newLayout); }, [layout, saveLayout] ); const handleVerticalLayout = useCallback( (sizes: number[]) => { + if (!layout) return; const newLayout = { ...layout, app: sizes[0], build: sizes[1] }; setLayout(newLayout); saveLayout(newLayout); }, [layout, saveLayout] );
98-117: Verify panel size calculations and constraints.The layout uses
defaultSizeprops that referencelayout.chat,layout.app, andlayout.build. Ensure that:
- The horizontal split correctly accounts for the full width (line 98:
chat, line 102:100 - layout.chat)- The vertical split sizes (
appandbuild) sum to 100 when persisted- The
maxSize={50}constraint on the build panel (line 117) doesn't conflict with persisted layouts wherelayout.build > 50Consider adding validation when loading the layout to ensure the values are within valid ranges:
const loadLayout = async () => { if (!builderId) { setLayout(DEFAULT_LAYOUT); setIsLoading(false); return; } try { const rtInfo = await RpcApi.GetRTInfoCommand(TabRpcClient, { oref: `builder:${builderId}`, }); if (rtInfo?.["builder:layout"]) { const loadedLayout = rtInfo["builder:layout"] as Record<string, number>; // Validate layout values const validatedLayout = { chat: Math.max(20, Math.min(80, loadedLayout.chat)), app: Math.max(20, Math.min(80, loadedLayout.app)), build: Math.max(20, Math.min(50, loadedLayout.build)), // respect maxSize }; setLayout(validatedLayout); } else { setLayout(DEFAULT_LAYOUT); } } catch (error) { console.error("Failed to load builder layout:", error); setLayout(DEFAULT_LAYOUT); } finally { setIsLoading(false); } };
38-38: ****The review comment is based on a misunderstanding of the RPC architecture.
TabRpcClientis not tab-specific despite its name—it is the single, application-wide RPC client used throughout the frontend (in wave.ts, app panels, workspace, views, etc.). There is no separateBuilderRpcClientbecause the architecture uses oref-based scoping (e.g.,builder:${builderId}) to route requests to the appropriate backend resource, not separate client instances. The code correctly usesTabRpcClientwith builder-scoped oref values, which is the intended pattern.Likely an incorrect or invalid review comment.
1 participant
No description provided.