fix tsunami scaffold in build #2564
Conversation
…eature flag to show the waveapp builder. new env var to pass the Wave.app resources path
WalkthroughConsolidates tsunami app metadata into a single Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tsunami/build/build.go (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). (5)
🔇 Additional comments (5)
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)
frontend/app/workspace/widgets.tsx (2)
376-393: Missing feature flag check in non-supercompact Apps button rendering.Line 376 only checks
isDev()to determine if the Apps button should be rendered in non-supercompact mode, but it should also checkfeatureWaveAppBuilderfor consistency with the supercompact layout (lines 348-360).Apply this diff to add the missing feature flag check:
- {isDev() ? ( + {isDev() || featureWaveAppBuilder ? ( <div ref={appsButtonRef} className="flex flex-col justify-center items-center w-full py-1.5 pr-0.5 text-secondary text-lg overflow-hidden rounded-sm hover:bg-hoverbg hover:text-white cursor-pointer"
411-417: Floating window render guard should include feature flag.The
AppsFloatingWindowis only rendered whenisDev()is true (line 411), but it should also be accessible whenfeatureWaveAppBuilderis enabled for consistency with the button visibility logic.Apply this diff to include the feature flag:
- {isDev() && appsButtonRef.current && ( + {(isDev() || featureWaveAppBuilder) && appsButtonRef.current && ( <AppsFloatingWindowtsunami/build/build.go (1)
576-584: Potential nil dereference inTsunamiBuild/TsunamiRunon early errorsBoth
TsunamiBuildandTsunamiRundo:buildEnv, err := TsunamiBuildInternal(opts) defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) if err != nil { return err }But
TsunamiBuildInternalreturnsnil, errfor several early failures (e.g., bad app path, bad scaffold path, environment verification failure). In those cases, thedefer buildEnv.cleanupTempDir(...)will be scheduled with a nil receiver and will panic when invoked.Consider guarding the defer with a nil check (and similarly gating
setupSignalCleanup):func TsunamiBuild(opts BuildOpts) error { - buildEnv, err := TsunamiBuildInternal(opts) - defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) - if err != nil { - return err - } - setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) - return nil + buildEnv, err := TsunamiBuildInternal(opts) + if buildEnv != nil { + defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + } + if err != nil { + return err + } + if buildEnv != nil { + setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + } + return nil } func TsunamiRun(opts BuildOpts) error { oc := opts.OutputCapture - buildEnv, err := TsunamiBuildInternal(opts) - defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + buildEnv, err := TsunamiBuildInternal(opts) + if buildEnv != nil { + defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + } if err != nil { return err } - setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + if buildEnv != nil { + setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + }This preserves existing behavior when
buildEnvis valid while avoiding panics on early failures.Also applies to: 971-975
🧹 Nitpick comments (2)
pkg/aiusechat/tools_tsunami.go (1)
22-31: Consider logging unmarshal errors.The helper function silently returns an empty string when
ReUnmarshalfails. While this provides a safe fallback, logging the error could help diagnose issues with malformed metadata during development or troubleshooting.Consider this enhancement:
func getTsunamiShortDesc(rtInfo *waveobj.ObjRTInfo) string { if rtInfo == nil || rtInfo.TsunamiAppMeta == nil { return "" } var appMeta wshrpc.AppMeta - if err := utilfn.ReUnmarshal(&appMeta, rtInfo.TsunamiAppMeta); err == nil && appMeta.ShortDesc != "" { + if err := utilfn.ReUnmarshal(&appMeta, rtInfo.TsunamiAppMeta); err != nil { + log.Printf("warning: failed to unmarshal tsunami appmeta: %v", err) + return "" + } + if appMeta.ShortDesc != "" { return appMeta.ShortDesc } return "" }pkg/blockcontroller/tsunamicontroller.go (1)
51-55: Consider logging the error when manifest reading fails.The function silently returns when
ReadAppManifestfails. While this may be intentional (since metadata is optional), logging the error would help with debugging when apps don't display expected metadata.Apply this diff to add error logging:
func (c *TsunamiController) setManifestMetadata(appId string) { manifest, err := waveappstore.ReadAppManifest(appId) if err != nil { + log.Printf("TsunamiController: failed to read manifest for app %s: %v", appId, err) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
Taskfile.yml(2 hunks)docs/docs/releasenotes.mdx(1 hunks)electron-builder.config.cjs(2 hunks)emain/emain-menu.ts(3 hunks)emain/emain-platform.ts(2 hunks)emain/emain-util.ts(1 hunks)emain/emain-wavesrv.ts(3 hunks)frontend/app/view/tsunami/tsunami.tsx(1 hunks)frontend/app/workspace/widgets.tsx(2 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/aiusechat/tools_tsunami.go(5 hunks)pkg/blockcontroller/tsunamicontroller.go(4 hunks)pkg/waveapputil/waveapputil.go(2 hunks)pkg/wavebase/wavebase.go(4 hunks)pkg/waveobj/objrtinfo.go(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)schema/settings.json(1 hunks)tsunami/build/build.go(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/tools_tsunami.go
🧬 Code graph analysis (11)
emain/emain-util.ts (1)
pkg/wavebase/wavebase.go (1)
WaveAppResourcesPathVarName(32-32)
emain/emain-wavesrv.ts (3)
emain/emain-util.ts (1)
WaveAppResourcesPathVarName(8-8)pkg/wavebase/wavebase.go (1)
WaveAppResourcesPathVarName(32-32)emain/emain-platform.ts (1)
getElectronAppResourcesPath(273-273)
emain/emain-platform.ts (1)
frontend/app/store/global.ts (1)
isDev(842-842)
frontend/types/gotypes.d.ts (5)
pkg/wshrpc/wshrpctypes.go (1)
AppMeta(1057-1062)tsunami/engine/clientimpl.go (1)
AppMeta(43-48)tsunami/app/defaultclient.go (1)
AppMeta(22-22)tsunami/demo/cpuchart/app.go (1)
AppMeta(12-15)tsunami/demo/pomodoro/app.go (1)
AppMeta(11-14)
pkg/waveapputil/waveapputil.go (1)
pkg/wavebase/wavebase.go (1)
GetWaveAppResourcesPath(121-123)
pkg/aiusechat/tools_tsunami.go (3)
pkg/waveobj/objrtinfo.go (1)
ObjRTInfo(6-27)pkg/wshrpc/wshrpctypes.go (1)
AppMeta(1057-1062)pkg/util/utilfn/marshal.go (1)
ReUnmarshal(36-42)
pkg/blockcontroller/tsunamicontroller.go (6)
pkg/waveappstore/waveappstore.go (1)
ReadAppManifest(722-744)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/wshrpc/wshrpctypes.go (1)
AppMeta(1057-1062)pkg/wstore/wstore_rtinfo.go (1)
SetRTInfo(84-125)pkg/wps/wps.go (1)
Broker(46-50)pkg/wps/wpstypes.go (2)
WaveEvent(26-32)Event_TsunamiUpdateMeta(23-23)
emain/emain-menu.ts (3)
pkg/wconfig/settingsconfig.go (1)
FullConfigType(259-269)frontend/app/store/wshclientapi.ts (1)
RpcApi(677-677)emain/emain-wsh.ts (1)
ElectronWshClient(116-116)
pkg/wavebase/wavebase.go (1)
emain/emain-util.ts (1)
WaveAppResourcesPathVarName(8-8)
tsunami/build/build.go (1)
tsunami/build/buildutil.go (1)
DirFS(14-17)
frontend/app/workspace/widgets.tsx (2)
emain/emain-platform.ts (1)
isDev(280-280)frontend/app/store/global.ts (1)
isDev(842-842)
⏰ 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: merge-gatekeeper
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (36)
docs/docs/releasenotes.mdx (1)
15-15: Documentation update looks good.The new Thinking Mode Toggle feature bullet is clearly written, properly positioned in the Wave AI Updates section, and consistent with the surrounding release notes format and style.
pkg/waveobj/objrtinfo.go (1)
7-8: LGTM! Clean consolidation of tsunami metadata.The refactoring from individual fields (TsunamiTitle, TsunamiShortDesc, TsunamiIcon, TsunamiIconColor) to a single TsunamiAppMeta field simplifies the structure and aligns with the AppMeta type used throughout the codebase.
pkg/wconfig/metaconsts.go (1)
15-16: LGTM! Feature flag constant properly defined.The new configuration key follows the established naming convention and aligns with the corresponding schema and settings changes.
frontend/app/view/tsunami/tsunami.tsx (1)
65-67: LGTM! Properly adapted to consolidated metadata structure.The change from building an AppMeta object from individual fields to directly using
rtInfo["tsunami:appmeta"]aligns with the backend refactoring and simplifies the code.pkg/aiusechat/tools_tsunami.go (1)
41-43: LGTM! Consistent usage of the helper function.All call sites have been properly updated to use
getTsunamiShortDesc(rtInfo)instead of direct field access, maintaining backward compatibility with the consolidated metadata structure.Also applies to: 127-129, 152-154, 190-192
pkg/wconfig/settingsconfig.go (1)
61-62: LGTM! Feature flag field properly integrated.The new
FeatureWaveAppBuilderfield follows the established conventions with appropriate JSON tags and positioning within the struct.schema/settings.json (1)
23-25: LGTM! Schema properly updated for the feature flag.The schema addition correctly defines the
feature:waveappbuilderproperty as a boolean, maintaining consistency with the Go struct definition.frontend/app/workspace/widgets.tsx (2)
232-232: LGTM! Feature flag properly initialized.The feature flag is correctly read from settings with an appropriate default value of
false.
346-360: LGTM! Supercompact layout correctly checks the feature flag.The rendering logic properly considers the
featureWaveAppBuilderflag alongsideisDev()andshowHelpfor controlling visibility in supercompact mode.frontend/types/gotypes.d.ts (2)
917-918: LGTM! Type definitions properly updated for consolidated metadata.The
ObjRTInfotype correctly reflects the backend change, replacing individual tsunami fields with a singletsunami:appmetaproperty of typeAppMeta.
1030-1030: LGTM! Feature flag added to settings type.The
SettingsTypecorrectly includes the newfeature:waveappbuilderboolean property, matching the backend schema.pkg/blockcontroller/tsunamicontroller.go (3)
150-152: LGTM!The placement and conditional call to
setManifestMetadatais appropriate. Setting metadata early in the Start flow ensures it's available before the app runs.
189-189: Good addition for debugging build failures.Logging the BuildOpts structure will help diagnose build issues more effectively.
51-76: The removedfetchAndSetSchemasmethod has no remaining references in the codebase.Verification confirms complete removal with no external dependencies. The codebase is safe to use with the new manifest-based approach.
emain/emain-util.ts (1)
8-8: LGTM! Constant definition is consistent with Go backend.The new constant matches
WaveAppResourcesPathVarNameinpkg/wavebase/wavebase.goand follows the existing naming convention.emain/emain-platform.ts (3)
152-153: LGTM! Helpful clarification comment.The comment clarifies the dev mode path behavior for
import.meta.dirname.
160-166: LGTM! Resources path correctly distinguishes dev from production.The function properly handles the difference between development and production environments:
- Dev: Returns the app base path (same as
getElectronAppBasePath())- Production: Returns
process.resourcesPath, which points to the Electron resources directory (parent of app.asar)This aligns with electron-builder's
extraResourcesconfiguration for items like tsunami scaffold that need to be outside the asar bundle.
273-273: LGTM! Function properly exported.emain/emain-wavesrv.ts (2)
11-11: LGTM! Imports are correctly organized.The imports properly distinguish between:
getElectronAppResourcesPathfromemain-platform(function implementation)WaveAppResourcesPathVarNamefromemain-util(constant definition)Also applies to: 21-26
68-68: LGTM! Environment variable correctly passed to wavesrv.The resources path is properly set in the environment before spawning the wavesrv process, allowing the Go backend to access and cache this value.
pkg/waveapputil/waveapputil.go (1)
21-28: LGTM! Tsunami scaffold path correctly uses resources path.The change from app path to resources path aligns with the broader refactoring to use
extraResourcesin electron-builder. This ensures the tsunami scaffold is accessible as regular filesystem files outside the asar bundle.pkg/wavebase/wavebase.go (4)
32-32: LGTM! Constant matches TypeScript definition.The constant name and value match the TypeScript version in
emain/emain-util.ts, ensuring cross-language consistency.
54-54: LGTM! Cache variable follows existing pattern.
103-104: LGTM! Environment variable properly cached and cleaned up.The caching and unsetting follow the established pattern for other environment variables like
WaveAppPathVarName, ensuring consistent environment cleanup after startup.
121-123: LGTM! Getter implementation is clean and consistent.The getter follows the same simple pattern as
GetWaveAppPath()and other path getters in this file.emain/emain-menu.ts (2)
319-326: LGTM! Clean refactor to fetch full config.The change from fetching specific config values to fetching the full config is well-implemented:
- Maintains existing
fullscreenOnLaunchextraction logic- Error handling preserves current behavior
- Optional chaining protects against null values
- Enables feature flag checking without additional RPC calls
113-135: No issues found - code is correct as written.The feature flag implementation is type-safe. TypeScript automatically includes
.d.tsfiles (likegotypes.d.ts) withdeclare globalas ambient declarations, makingFullConfigTypeavailable globally without explicit import. The"feature:waveappbuilder"setting is explicitly typed asbooleaninSettingsType(line 1030 offrontend/types/gotypes.d.ts), so using it directly in a boolean context is safe. The implementation matches the codebase's existing pattern for accessing and using settings values.Taskfile.yml (2)
528-548: Unix scaffold: node_modules → nm move looks correctThe added
mv scaffold/node_modules scaffold/nmfits the precedingcd scaffold && npm installand the taskdir: tsunami/frontend: the path is correct,scaffoldis freshly recreated, and this will deterministically rename the installednode_modulestonmwith no stale state. Matches the repo-wide nm convention for the tsunami scaffold.
549-568: Windows scaffold: Move-Item mirrors Unix behaviorThe PowerShell
Move-Item -Path scaffold/node_modules -Destination scaffold/nmcorrectly mirrors the Unix mv semantics relative todir: tsunami/frontendand the earlierSet-Location scaffold; npm installcall. This keeps the scaffold layout consistent across platforms and in line with the nm convention.electron-builder.config.cjs (1)
21-39: Packaging tsunamiscaffold via extraResources is consistent; verify runtime lookup pathExcluding
tsunamiscaffoldfrom the maindistfiles and then addingdist/tsunamiscaffold → tsunamiscaffoldinextraResourcesis a clean way to keep the scaffold outside the asar and under the app’s resources root, while avoiding duplicate copies. This aligns with the new scaffold layout (usingnm) and the dedicated packaging of tsunami assets.The only thing to double-check is that whatever code resolves the tsunami scaffold path now uses the Electron resources directory (e.g.,
process.resourcesPath/tsunamiscaffoldor the new env wiring) rather than assuming it lives underapp.asar(.unpacked)/dist/tsunamiscaffold.tsunami/build/build.go (6)
110-115: Centralizing Go binary path inBuildEnvlooks goodStoring the resolved Go executable path on
BuildEnvand returning it fromverifyEnvironmentensuresgo mod tidyandgo buildconsistently use the same tool, including when a customGoPathis provided. No issues spotted here.Also applies to: 331-335
338-446:createGoModcorrectly usesBuildEnvfor Go version andgo mod tidyPassing
buildEnv *BuildEnvintocreateGoModand usingbuildEnv.GoVersionforAddGoStmtplusbuildEnv.GoPathforgo mod tidykeeps module metadata and tooling in sync with the Go version that passed validation. The extraoc.Flush()on tidy failure is also helpful for surfacing errors. The function now has a clear precondition thatbuildEnvis non‑nil and comes fromverifyEnvironment; given current call sites, this looks safe.
479-513: Scaffold verification fornmdirectory matches new layoutUpdating
verifyScaffoldFsto require annmdirectory (and still enforcing that it’s an actual directory) aligns with the new scaffold structure wherenmrepresentsnode_modules. This keeps early validation in sync with the copy logic later in the build.
576-691: Build flow updates to passBuildEnvinto go module/build steps are coherentPlumbing
buildEnvintoTsunamiBuildInternal, including the new debug log for the scaffold path and updated calls tocreateGoMod(..., buildEnv, ...)andrunGoBuild(tempDir, buildEnv, opts), keeps all Go tool invocations tied to the environment verified up front. The added debug output around scaffold and temp dir paths should also make diagnosing build issues easier.
750-810: UsingbuildEnv.GoPathforgo buildkeeps tooling consistent
runGoBuildnow invokesexec.Command(buildEnv.GoPath, ...)instead of assuminggoon PATH, which matches the result ofCheckGoVersionand respects a custom Go path. The rest of the function (output path resolution, presence check for.gofiles, and logging) remains unchanged and correct.
841-883: Tailwind command changes align with new Node setup but should be verified across environmentsSwitching to
opts.getNodePath()and adding--preserve-symlinks-main/--preserve-symlinksbefore the Tailwind CLI entrypoint is consistent with the newnm→node_modulesbehavior and should help avoid module‑resolution issues when using symlinks. The command structure (node [flags] script -i ... -o ...) andELECTRON_RUN_AS_NODE=1env look reasonable, but I’d recommend confirming this still works on all supported Node/Electron combinations and platforms (especially older Node versions) in your CI or local testing.
No description provided.