more work on tsunami builder #2534
Conversation
…build scaffold dir and put into dist)
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThis PR adds Tsunami configuration keys and scaffold build support, updates build tooling and Taskfile tasks to produce a dist/tsunamiscaffold, extends Electron menu and IPC to support workspace/builder-specific app menus, adds secret declaration and manifest generation to the Tsunami engine and client, moves several build/runtime Go options to configurable settings (SDK version, Go path), adjusts AI/OpenAI endpoint handling, and makes multiple frontend adjustments (context menus, AI panel behavior, types). Several documentation files were removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention during review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
emain/emain-menu.ts (1)
33-43: Fix Wave window webContents detectionBecause
WaveBrowserWindowextendselectron.BrowserWindow, the first branch always matches Wave windows and returns the shellBrowserWindow.webContents. As a result, “Reload Tab”, zoom, context menu click routing, etc. now hit the wrong target and no longer reach the active tab’sBrowserView. Please check for the Wave subclass first so we can keep targeting the tab view, and only fall back to the generic BrowserWindow branch for Tsunami builder windows.function getWindowWebContents(window: electron.BaseWindow): electron.WebContents { if (window == null) { return null; } - // Check BrowserWindow first (for Tsunami Builder windows) - if (window instanceof electron.BrowserWindow) { - return window.webContents; - } - // Check WaveBrowserWindow (for main Wave windows with tab views) - if (window instanceof WaveBrowserWindow) { - if (window.activeTabView) { - return window.activeTabView.webContents; - } - return null; - } + if (window instanceof WaveBrowserWindow) { + return window.activeTabView?.webContents ?? null; + } + if (window instanceof electron.BrowserWindow) { + return window.webContents; + } return null; }tsunami/build/build.go (2)
553-564: Avoid self-referential UI symlink when no SDK replace pathWhen
SdkReplacePathis empty (the new default when building against a released SDK version),filepath.Join("", "ui")produces the relative target"ui". The symlink attempDir/uithen points back to itself, so any attempt to importgithub.com/wavetermdev/waveterm/tsunami/uiblows up with “too many levels of symbolic links,” breaking builds that rely on the module version instead of a local replace.Only create this symlink when we actually have a real replace path (or skip entirely when using the module). One way to fix it:
- uiTargetPath := filepath.Join(opts.SdkReplacePath, "ui") - if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { + if opts.SdkReplacePath == "" { + if opts.Verbose { + oc.Printf("Skipping UI symlink creation - no SdkReplacePath configured") + } + } else { + uiTargetPath := filepath.Join(opts.SdkReplacePath, "ui") + if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { return buildEnv, fmt.Errorf("failed to create ui symlink: %w", err) } - if opts.Verbose { - oc.Printf("Created UI symlink: %s -> %s", uiLinkPath, uiTargetPath) - } + if opts.Verbose { + oc.Printf("Created UI symlink: %s -> %s", uiLinkPath, uiTargetPath) + } + }
312-322: HonorGoPathwhen spawning Go toolsWe now surface
BuildOpts.GoPath, but bothgo mod tidyand the finalgo buildstill invoke"go"directly. Anyone relying ontsunami:gopath(or the watcher-resolved custom path) passesverifyEnvironment, yet the subsequent commands blow up because that binary isn’t on PATH.Please thread the resolved executable through every Go invocation, e.g.:
- tidyCmd := exec.Command("go", "mod", "tidy") + goExe := opts.GoPath + if goExe == "" { + goExe = "go" + } + tidyCmd := exec.Command(goExe, "mod", "tidy") ... - buildCmd := exec.Command("go", args...) + goExe := opts.GoPath + if goExe == "" { + goExe = "go" + } + buildCmd := exec.Command(goExe, args...)Reuse the same helper for any other Go subprocess so the new configuration knob actually works.
Also applies to: 664-672
🧹 Nitpick comments (2)
frontend/app/aipanel/aimessage.tsx (1)
52-57: Approve the layout stability improvement.The fixed height (
h-[3lh]) prevents layout shift during streaming content, which improves the user experience. The container is already conditionally rendered at a higher level (lines 245-252), so it only appears when appropriate.Note that when
displayTextis empty (e.g., waiting for tool approvals), this will show a 3-line-height empty space. If this is visually undesirable, consider applying the fixed height only when content is present.Optional refinement:
- <div - ref={scrollRef} - className="text-sm text-gray-500 overflow-y-auto h-[3lh] max-w-[600px] pl-9" - > + <div + ref={scrollRef} + className={cn( + "text-sm text-gray-500 overflow-y-auto max-w-[600px] pl-9", + displayText ? "h-[3lh]" : "max-h-[3lh]" + )} + > {displayText} </div>frontend/builder/builder-buildpanel.tsx (1)
39-39: Event handlers properly wired; note unused ref.The
preRefis declared but not currently used. If it's intended for future enhancements, consider adding a comment; otherwise, it can be removed to reduce clutter.Also applies to: 80-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.vscode/settings.json(1 hunks)Taskfile.yml(3 hunks)electron-builder.config.cjs(1 hunks)emain/emain-menu.ts(13 hunks)emain/preload.ts(1 hunks)frontend/app/aipanel/aimessage.tsx(1 hunks)frontend/app/aipanel/waveai-model.tsx(1 hunks)frontend/app/store/contextmenu.ts(1 hunks)frontend/app/tab/tabbar.tsx(1 hunks)frontend/builder/builder-buildpanel.tsx(2 hunks)frontend/types/custom.d.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)go.mod(1 hunks)pkg/aiusechat/openai/openai-convertmessage.go(2 hunks)pkg/aiusechat/tsunami/global-keyboard-handling.md(0 hunks)pkg/aiusechat/tsunami/graphing.md(0 hunks)pkg/aiusechat/tsunami/system.md(0 hunks)pkg/aiusechat/uctypes/usechat-types.go(1 hunks)pkg/aiusechat/usechat.go(2 hunks)pkg/buildercontroller/buildercontroller.go(3 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)schema/settings.json(1 hunks)tsunami/app/atom.go(1 hunks)tsunami/app/defaultclient.go(1 hunks)tsunami/build/build.go(5 hunks)tsunami/cmd/main-tsunami.go(3 hunks)tsunami/engine/clientimpl.go(4 hunks)tsunami/templates/app-main.go.tmpl(2 hunks)
💤 Files with no reviewable changes (3)
- pkg/aiusechat/tsunami/global-keyboard-handling.md
- pkg/aiusechat/tsunami/graphing.md
- pkg/aiusechat/tsunami/system.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 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:
pkg/aiusechat/openai/openai-convertmessage.gopkg/aiusechat/usechat.go
📚 Learning: 2025-10-14T06:30:54.763Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 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/aimessage.tsx
📚 Learning: 2025-11-01T00:57:23.025Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2504
File: frontend/app/aipanel/aipanel-contextmenu.ts:15-16
Timestamp: 2025-11-01T00:57:23.025Z
Learning: In the waveterm codebase, types defined in custom.d.ts are globally available and do not require explicit imports. Backend types defined in gotypes.d.ts are also globally available.
Applied to files:
frontend/types/gotypes.d.ts
🧬 Code graph analysis (9)
pkg/aiusechat/openai/openai-convertmessage.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
DefaultAIEndpoint(12-12)
frontend/builder/builder-buildpanel.tsx (2)
frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(43-565)frontend/app/store/contextmenu.ts (1)
ContextMenuModel(68-68)
tsunami/app/defaultclient.go (2)
tsunami/engine/clientimpl.go (2)
SecretMeta(48-51)GetDefaultClient(115-117)tsunami/app/atom.go (1)
SecretMeta(26-29)
pkg/aiusechat/usechat.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
DefaultAIEndpoint(12-12)
frontend/app/store/contextmenu.ts (1)
frontend/app/store/global.ts (2)
atoms(816-816)getApi(826-826)
pkg/buildercontroller/buildercontroller.go (3)
pkg/wconfig/filewatcher.go (1)
GetWatcher(33-57)pkg/wavebase/wavebase.go (1)
GetWaveAppPath(113-115)tsunami/cmd/main-tsunami.go (1)
TsunamiSdkVersion(17-17)
pkg/wconfig/settingsconfig.go (1)
tsunami/cmd/main-tsunami.go (1)
TsunamiSdkVersion(17-17)
tsunami/engine/clientimpl.go (3)
tsunami/app/atom.go (1)
SecretMeta(26-29)tsunami/app/defaultclient.go (2)
DeclareSecret(166-176)PrintAppManifest(178-181)tsunami/engine/schema.go (2)
GenerateConfigSchema(295-298)GenerateDataSchema(301-304)
emain/emain-menu.ts (6)
emain/emain-platform.ts (1)
unamePlatform(273-273)emain/emain-tabview.ts (1)
clearTabCache(215-221)emain/emain-window.ts (2)
getAllWaveWindows(652-654)getWaveWindowByWorkspaceId(644-650)emain/emain-builder.ts (2)
focusedBuilderWindow(20-20)getBuilderWindowById(22-24)frontend/app/store/wshclientapi.ts (1)
RpcApi(632-632)emain/emain-wsh.ts (1)
ElectronWshClient(116-116)
⏰ 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). (5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (17)
pkg/aiusechat/openai/openai-convertmessage.go (2)
148-150: LGTM! Useful debug logging.The conditional endpoint logging helps developers identify when non-default endpoints are being used, which is valuable for debugging custom configurations.
319-323: LGTM! Clear request type identification.The header logic correctly distinguishes builder requests from regular AI requests, which will help with backend routing and analytics.
pkg/aiusechat/uctypes/usechat-types.go (1)
12-12: Good refactoring to centralize the endpoint definition.Moving the default AI endpoint to a shared constant in the types package provides a single source of truth and improves maintainability.
pkg/aiusechat/usechat.go (2)
100-100: LGTM! Correct usage of the centralized constant.The change properly uses the newly centralized
uctypes.DefaultAIEndpointconstant, completing the refactoring to provide a single source of truth for the default endpoint.
689-691: Verify that empty system prompt is intended for builder mode.When
BuilderIdis present, the system prompt is now initialized to an empty slice. This is a significant change from the previous behavior where builder mode had specific system prompt text (BuilderSystemPromptText_OpenAI + tsunamiSystemDoc, per the summary).Please confirm this is the intended behavior for the tsunami builder system. If system instructions for builders are being provided through a different mechanism (e.g., per-conversation or via tools), that's fine. Otherwise, the AI may lack important context about its role and capabilities in builder mode.
frontend/app/tab/tabbar.tsx (1)
634-635: Aligned with new workspace menu flow.Switching to
showWorkspaceAppMenuroutes the ellipsis button through the dedicated workspace menu channel exposed by the preload bridge, so the renderer no longer needs to juggle ad-hoc context menu payloads. Looks good.emain/preload.ts (1)
21-22: Preload surface covers the new menu IPC.These bridge helpers keep renderer code within the blessed API while enabling the per-workspace and builder menu flows you introduced in the main process.
frontend/types/custom.d.ts (2)
90-91: LGTM! Clean API additions for workspace and builder menus.The new methods follow the existing API patterns and provide clear separation of concerns for different menu contexts.
92-92: All call sites properly updated for breaking change.The
menuparameter is now required across all 9 call sites in the codebase, confirming the breaking API change has been implemented consistently:
- frontend/builder/builder-buildpanel.tsx, frontend/app/workspace/widgets.tsx, frontend/app/tab/tab.tsx, frontend/app/view/preview/preview-model.tsx, frontend/app/view/preview/preview-directory.tsx (2 calls), frontend/app/app.tsx, frontend/app/block/blockframe.tsx, and frontend/app/aipanel/aipanel-contextmenu.ts all pass the menu parameter.
tsunami/cmd/main-tsunami.go (1)
17-17: LGTM! Clean SDK version constant usage.The SDK version is properly centralized as a constant and consistently applied to both build and run command paths.
Also applies to: 77-77, 106-106
tsunami/app/atom.go (1)
25-29: LGTM! Clean metadata structure for secrets.The
SecretMetatype follows the existing patterns in the file and provides appropriate fields for secret documentation and validation.frontend/app/aipanel/waveai-model.tsx (1)
302-318: LGTM! Backward-compatible enhancement with clear logic.The optional
newLineparameter provides flexibility while maintaining backward compatibility. The logic correctly handles both newline and space insertion based on the current input state.tsunami/app/defaultclient.go (2)
166-176: LGTM! Clean secret declaration wrapper.The function properly handles nil metadata, maps to the engine layer, and returns the environment variable value. The implementation follows existing patterns in the file.
178-181: LGTM! Simple delegation function.Clean wrapper that follows the existing delegation pattern in the file.
schema/settings.json (1)
253-268: LGTM! Clean schema additions for tsunami configuration.The new properties are well-typed and follow the existing schema patterns. The wildcard property allows for future extensibility.
frontend/builder/builder-buildpanel.tsx (2)
11-33: LGTM! Well-structured context menu integration.The context menu implementation provides a clean way to add build output to the AI context. The markdown formatting with code blocks and the "from builder output:" label provide good context for the AI.
54-72: LGTM! Efficient copy-on-select implementation.The debounced copy-on-select feature enhances UX for build output. The 50ms debounce prevents excessive clipboard operations, and the useCallback hooks optimize performance.
|
|
||
| const workspace = globalStore.get(atoms.workspace); | ||
| let oid: string; | ||
|
|
||
| if (workspace != null) { | ||
| oid = workspace.oid; | ||
| } else { | ||
| oid = globalStore.get(atoms.builderId); | ||
| } | ||
|
|
||
| getApi().showContextMenu(oid, electronMenuItems); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify builderId is always defined in builder context.
The fallback logic correctly handles both workspace and builder contexts. However, ensure that builderId is always defined when workspace is null, or add a defensive check.
🏁 Script executed:
#!/bin/bash
# Description: Verify builderId atom usage and initialization
# Search for builderId atom initialization and usage
rg -nP --type=ts -C3 '\bbuilderId\s*:.*Atom'Length of output: 1112
---
🏁 Script executed:
```shell
#!/bin/bash
# Find builderIdAtom definition and initialization
# Search for builderIdAtom definition
rg -nP --type=ts -C5 '\bbuilderIdAtom\s*=' | head -50
# Search for builderIdAtom in wave.ts specifically
echo "---"
rg -nP --type=ts 'builderIdAtom' | grep -i 'wave\.ts'
Length of output: 897
🏁 Script executed:
#!/bin/bash
# Find where builderId is initialized and used
fd -e ts -e tsx | xargs grep -l "builderId" | grep -E '(wave|init|main|boot|startup)' || echo "No matches in startup files"Length of output: 133
---
🏁 Script executed:
```shell
#!/bin/bash
# Find initGlobalAtoms call sites
rg -nP --type=ts 'initGlobalAtoms\s*\(' -A5 | head -40
# Check contextmenu.ts for builderId handling around line 58
rg -nP --type=ts -B10 -A5 'showContextMenu' frontend/app/store/contextmenu.ts
Length of output: 1712
---
🏁 Script executed:
```shell
#!/bin/bash
# Find initGlobalStoreContext and its call sites
rg -nP --type=ts 'initGlobalStoreContext' -B2 -A10
# Check what the ContextMenuModel.showContextMenu expects
rg -nP --type=ts 'showContextMenu.*oid' -B5 -A5
Length of output: 2594
---
🏁 Script executed:
```shell
#!/bin/bash
# Find GlobalInitOptions type definition
rg -nP --type=ts 'interface GlobalInitOptions|type GlobalInitOptions' -A10
Length of output: 627
Add null check for builderId in fallback logic.
The fallback at line 59 can assign an undefined value to oid. Since builderId is optional in GlobalInitOptions (line 43 of global.ts), and the API at line 92 of custom.d.ts requires a string parameter for showContextMenu, add a defensive guard:
oid = globalStore.get(atoms.builderId) ?? '';Or log an error if both workspace and builderId are undefined, indicating an initialization problem.
🤖 Prompt for AI Agents
In frontend/app/store/contextmenu.ts around lines 52 to 62, the fallback assigns
builderId which may be undefined causing a non-string to be passed to
getApi().showContextMenu; change the fallback to defensively handle undefined by
using a safe default or error path (e.g., set oid =
globalStore.get(atoms.builderId) ?? '' or check both workspace and builderId and
log/throw an error if neither is present) so that showContextMenu always
receives a string and initialization problems are surfaced.
…icks up their classes automatically. remove brittle inline decls...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tsunami/build/build.go (3)
73-75: Document the new BuildOpts fields.The new
SdkVersionandGoPathfields lack documentation. Consider adding comments to clarify:
SdkVersion: Expected format (e.g.,v0.1.0, semantic versioning)GoPath: Whether it should be an absolute path to the Go executable- The relationship between these fields and
SdkReplacePathApply this diff to add documentation:
+ // SdkVersion specifies the version of the Tsunami SDK to use (e.g., "v0.1.0") SdkVersion string + // GoPath is the absolute path to the Go executable (optional, auto-detected if empty) GoPath string
138-158: Validation logic is sound, consider enhancing the error message.The requirement for either
SdkVersionorSdkReplacePathis appropriate, and the Go path resolution logic (with custom path support and auto-detection fallback) is well-implemented with good logging.Consider making the error message more helpful:
if opts.SdkVersion == "" && opts.SdkReplacePath == "" { - return nil, fmt.Errorf("either SdkVersion or SdkReplacePath must be set") + return nil, fmt.Errorf("either SdkVersion or SdkReplacePath must be set in BuildOpts") }
277-289: Consider validating SdkVersion format.The use of
opts.SdkVersionmakes the build system more flexible. However, there's no validation thatSdkVersionis in a valid format (e.g., semantic versioning likev0.1.0). WhileverifyEnvironmentensures eitherSdkVersionorSdkReplacePathis set, an invalid version format could cause issues withgo mod tidy.Consider adding validation before line 277:
+ // Validate SdkVersion format if provided + if opts.SdkVersion != "" { + versionRegex := regexp.MustCompile(`^v\d+\.\d+\.\d+`) + if !versionRegex.MatchString(opts.SdkVersion) { + return fmt.Errorf("invalid SdkVersion format: %s (expected format: v0.1.0)", opts.SdkVersion) + } + } + // Add requirement for tsunami SDK if err := modFile.AddRequire("github.com/wavetermdev/waveterm/tsunami", opts.SdkVersion); err != nil {Alternatively, verify that the existing validation in
verifyEnvironmentis sufficient by checking ifmodfile.AddRequirehandles invalid versions gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Taskfile.yml(4 hunks)tsunami/build/build.go(5 hunks)tsunami/engine/errcomponent.go(1 hunks)tsunami/engine/render.go(0 hunks)tsunami/templates/tailwind.css(0 hunks)
💤 Files with no reviewable changes (2)
- tsunami/templates/tailwind.css
- tsunami/engine/render.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Taskfile.yml
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: merge-gatekeeper
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
tsunami/build/build.go (2)
98-133: LGTM! Go executable discovery logic is robust.The exported
FindGoExecutablefunction provides a reasonable fallback strategy, checking PATH first and then platform-specific common installation locations. The export allows external callers to reuse this logic.
305-308: LGTM! Logging improvements enhance debuggability.The updated verbose output clearly shows the configured SDK version and conditionally displays the replace directive, making it easier to understand the build configuration.
tsunami/engine/errcomponent.go (1)
13-22: The original review comment is based on incorrect information and should be dismissed.The search confirms that
renderErrorComponentis actively called fromrender.goat line 185 within thecallCFuncWithErrorGuardfunction's error handling defer block. The premise that "render.go no longer uses it" is factually incorrect. Additionally, no external packages import from the engine package, confirming this function is package-internal and appropriately non-exported. The design—keeping the function unexported as a private utility for panic error handling—is sound and requires no changes.Likely an incorrect or invalid review comment.
No description provided.