More Builder Improvements + Tsunami AppInit #2571
Conversation
sawka
commented
Nov 19, 2025
sawka
commented
- Kebab Menu + Switch App Functionality
- Updated Secrets Tab (still more to do, but closer)
- Better Error Handling (Switch to Secrets Tab)
- Add AppInit functionality to Tsunami (fixes initialization issues and races with embedded assets, and error handling)
- Tsunami - ListStaticFiles
- Tsunami - More Robust Manifest generation (even when there are extra stdout messages)
|
Caution Review failedThe pull request is closed. WalkthroughThe diff migrates builder appId storage from saved init options into a new BuilderWindowType.builderAppId field and RT info; adds a "do-refresh" IPC path and exposes preload.doRefresh; introduces StopBuilderCommand across RPC server, client, and frontend APIs; replaces the builder "Env" tab with a Secrets UI and modal plus secret-related RPCs and error handling; adjusts builder UI styling and adds a dev badge; and refactors the tsunami build/init flow: adds AppInit(fn) that returns error, AST validation for AppInit, changes scaffold/template copy behavior and manifest extraction, and adds static-file listing utilities. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
pkg/buildercontroller/buildercontroller.go (1)
156-156: Clarify or remove cryptic debug log.The log format
"*** BC %s"is unclear. If this is temporary debug code, consider removing it. If it's intended to remain, make it more descriptive with proper context.Apply this diff for a more descriptive log:
- log.Printf("*** BC %s\n", appId) + log.Printf("BuilderController[%s]: starting app %s\n", bc.builderId, appId)Or remove it entirely if it's temporary:
- log.Printf("*** BC %s\n", appId)frontend/builder/tabs/builder-previewtab.tsx (1)
33-89: Secret‑error handling and navigation to Secrets tab are wired correctlyDetecting
"ERR-SECRET"indisplayMsgand short‑circuiting to a dedicated “Secrets Required” view, then switching tabs viaBuilderAppPanelModel.setActiveTab("secrets"), is coherent with the new secrets flow. Only minor nit: substring matching on the raw message is a bit brittle long‑term but acceptable for now.emain/emain-builder.ts (1)
35-49: Consider isolating RT‑info failures from window creationRight now, a failure in
SetRTInfoCommandwill rejectcreateBuilderWindowand prevent the window from appearing. If you want the builder window to still open even when RT‑info cannot be persisted, you could wrap this block in atry/catchand log instead of propagating.frontend/builder/store/builder-apppanel-model.ts (1)
214-229: switchBuilderApp flow is reasonable, but relies on fixed delaysStopping the builder, clearing
"builder:appid"viaSetRTInfoCommand, syncing the main process withsetBuilderWindowAppId(null), and finally callingdoRefresh()is a coherent way to bounce back to the app‑selection state. The fixed 500 ms/100 ms waits are heuristic; if you see flakiness, consider wiring this to explicit status/route events instead.frontend/builder/builder-apppanel.tsx (1)
200-201: Guard secrets tab content whenhasSecretsis falseThe "Secrets" tab button is gated on
hasSecrets, but the content panel is only gated onactiveTab === "secrets". IfactiveTabever remains"secrets"whilehasSecretsbecomes false (e.g., manifest change), users could end up on a hidden tab. Either enforce inBuilderAppPanelModelthatactiveTabis never"secrets"whenhasSecretsAtomis false, or mirror the guard here:- <div className="w-full h-full" style={{ display: activeTab === "secrets" ? "block" : "none" }}> - <ErrorBoundary> - <BuilderEnvTab /> - </ErrorBoundary> - </div> + {hasSecrets && ( + <div className="w-full h-full" style={{ display: activeTab === "secrets" ? "block" : "none" }}> + <ErrorBoundary> + <BuilderEnvTab /> + </ErrorBoundary> + </div> + )}Also applies to: 320-328, 364-368
frontend/builder/tabs/builder-secrettab.tsx (3)
25-47: Clarify status/tooltip for invalid mappings
SecretRowdistinguishes between “not mapped” and “mapped & valid”, but the tooltip text always says “Secret Has a Valid Mapping” wheneverisMappedis true, even whenisValidis false (no checkmark, but also no warning icon or different tooltip).Consider explicitly handling the “mapped but not found in availableSecrets” case so users see that as an error/warning instead of a valid mapping. For example, add a separate branch with a warning icon + “Mapped to unknown secret” tooltip keyed off
isMapped && !isValid.
174-190: Avoid callingsetLocalBindingsinside render; derive fromsecretBindingsin an effectThe initialization:
if (!localBindings || Object.keys(localBindings).length === 0) { if (Object.keys(secretBindings).length > 0) { setLocalBindings({ ...secretBindings }); } }runs during render and mutates state, which is generally discouraged and can be brittle with future React behavior. It also makes the relationship between
builderStatus.secretbindingsandlocalBindingsless explicit.Recommend moving this into a
useEffectthat depends onsecretBindingsand only seedslocalBindingswhen they’re empty, e.g.:useEffect(() => { if (Object.keys(secretBindings).length > 0 && Object.keys(localBindings).length === 0) { setLocalBindings({ ...secretBindings }); } }, [secretBindings, localBindings]);This keeps state transitions in effects and makes updates predictable.
230-235: Consider dedupingavailableSecretswhen adding a new secret
handleSetAndMapunconditionally appendssecretName:setAvailableSecrets((prev) => [...prev, secretName]);If
SetSecretsCommandis retried or the user goes through the dialog multiple times for the same name, this can introduce duplicates inavailableSecrets, which may affect any future UI that enumerates it.Minor, but you can cheaply guard against this with a set-style update:
setAvailableSecrets((prev) => prev.includes(secretName) ? prev : [...prev, secretName], );tsunami/build/build.go (1)
1071-1169: Scaffold copy behavior is correct; small nit on duplicate JSON copiesThe revised
copyScaffoldFSdoes the right high-level things:
- Handles
nmvia symlink-or-copy.- Copies
dist/.- Always copies
app-main.go.tmpl→app-main.go.- Conditionally copies
app-init.go.tmpl→app-init.gowhenhasAppInitis true.- Then copies
*.md,*.json, andtailwind.cssviafs.Glob.Functionally this is fine, but note that
package.jsonandpackage-lock.jsonare copied twice: once explicitly inpackageFiles, and again when matching*.json. It’s harmless (the second copy overwrites the first, onlyfileCountis inflated), but if you care about efficiency or accurate stats you could exclude those from the globbed patterns or special-case them when iteratingmatches.
📜 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 (27)
Taskfile.yml(2 hunks)emain/emain-builder.ts(3 hunks)emain/emain-ipc.ts(3 hunks)emain/preload.ts(1 hunks)frontend/app/aipanel/thinkingmode.tsx(3 hunks)frontend/app/modals/modalregistry.tsx(2 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/builder/builder-app.tsx(2 hunks)frontend/builder/builder-apppanel.tsx(5 hunks)frontend/builder/builder-buildpanel.tsx(1 hunks)frontend/builder/builder-workspace.tsx(1 hunks)frontend/builder/store/builder-apppanel-model.ts(5 hunks)frontend/builder/tabs/builder-envtab.tsx(0 hunks)frontend/builder/tabs/builder-previewtab.tsx(2 hunks)frontend/builder/tabs/builder-secrettab.tsx(1 hunks)frontend/types/custom.d.ts(1 hunks)frontend/wave.ts(1 hunks)pkg/buildercontroller/buildercontroller.go(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)tsunami/app/defaultclient.go(5 hunks)tsunami/build/build-ast.go(1 hunks)tsunami/build/build.go(10 hunks)tsunami/build/buildutil.go(1 hunks)tsunami/engine/clientimpl.go(4 hunks)tsunami/templates/app-init.go.tmpl(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/builder/tabs/builder-envtab.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
emain/emain-builder.tsemain/emain-ipc.ts
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshclient/wshclient.go
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 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/builder/store/builder-apppanel-model.ts
🧬 Code graph analysis (13)
frontend/builder/tabs/builder-previewtab.tsx (1)
frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(21-316)
frontend/app/store/wshclientapi.ts (2)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(364-370)
emain/emain-builder.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)emain/emain-wsh.ts (1)
ElectronWshClient(116-116)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
StopBuilderCommand(551-553)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(364-370)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
StopBuilderCommand(551-553)pkg/wshrpc/wshclient/wshclient.go (1)
StopBuilderCommand(662-665)pkg/buildercontroller/buildercontroller.go (1)
GetOrCreateController(72-89)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
StopBuilderCommand(551-553)pkg/wshrpc/wshclient/wshclient.go (1)
StopBuilderCommand(662-665)
tsunami/build/build-ast.go (1)
tsunami/build/build.go (1)
MainAppFileName(32-32)
frontend/builder/tabs/builder-secrettab.tsx (7)
frontend/app/element/tooltip.tsx (1)
Tooltip(143-176)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/modals/modal.tsx (1)
Modal(116-116)frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(21-316)frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/global.ts (1)
atoms(816-816)
tsunami/app/defaultclient.go (1)
tsunami/engine/clientimpl.go (1)
GetDefaultClient(116-118)
tsunami/engine/clientimpl.go (1)
tsunami/app/defaultclient.go (1)
RegisterAppInitFn(55-57)
pkg/buildercontroller/buildercontroller.go (1)
pkg/waveappstore/waveappstore.go (1)
BuildAppSecretEnv(804-842)
tsunami/build/build.go (1)
tsunami/build/buildutil.go (1)
CopyFileFromFS(153-188)
frontend/builder/store/builder-apppanel-model.ts (3)
frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)pkg/wshrpc/wshrpctypes.go (1)
BuilderStatusData(1077-1086)frontend/app/store/global.ts (3)
atoms(816-816)WOS(866-866)getApi(826-826)
⏰ 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 (32)
pkg/buildercontroller/buildercontroller.go (1)
305-310: LGTM! Error messages improved for better secret error identification.The addition of
(ERR-SECRET)markers makes it easier to identify and handle secret-related errors, which aligns well with the PR's objective of improved error handling and routing users to the Secrets Tab.tsunami/build/buildutil.go (1)
1-3: LGTM!Standard license header addition with no functional changes.
frontend/builder/builder-buildpanel.tsx (1)
88-88: LGTM!The addition of
rounded-br-2class provides consistent bottom-right border radius styling, coordinating with the parent container changes inbuilder-workspace.tsx.frontend/types/custom.d.ts (1)
130-130: LGTM!The
doRefreshmethod addition extends the Electron API to support window refresh functionality, appropriately using a void return type for this fire-and-forget operation.pkg/wshrpc/wshrpctypes.go (1)
346-346: LGTM!The
StopBuilderCommandinterface method is properly declared and well-positioned among related builder lifecycle methods. The signature is consistent with the implementation.frontend/builder/builder-workspace.tsx (2)
104-111: LGTM!The border radius styling update to 8px on the outer container coordinates well with the panel styling to ensure consistent visual appearance.
118-123: LGTM!The explicit
borderBottomRightRadius: 8style on the Panel ensures the build panel has consistent bottom-right corner rounding with its parent container.Taskfile.yml (2)
542-542: LGTM!Expanding the template copy pattern from a specific file to all
*.go.tmplfiles is more maintainable and supports the new AppInit functionality. This ensures all Go templates are included in the scaffold without requiring task file updates for each new template.
563-563: LGTM!The Windows version mirrors the Unix template copy expansion, ensuring cross-platform consistency in scaffold generation.
frontend/app/aipanel/thinkingmode.tsx (3)
48-48: Verify the hideQuick logic rationale.The condition
model.inBuilder && hasPremiumhides the "quick" thinking mode for builder users with premium access. This removes user choice and might be unexpected.Consider:
- Why should builder users with premium be prevented from using quick mode?
- Is there a performance/quality reason specific to builder context?
- Should this be a recommendation rather than enforced hiding?
The UX might be confusing if users in builder wonder why quick mode disappeared.
64-66: Conditional mapping is correct given the hideQuick behavior.The logic properly maps "quick" to "balanced" when
hideQuickis active, preventing an invalid UI state. However, this is contingent on the verification of thehideQuickrationale itself.
89-123: Implementation is technically correct.The filtering and rendering logic properly handles the conditional hiding of "quick" mode, including correct recalculation of
isFirstandisLastbased on the filtered array. The implementation is sound, though it depends on the verification of thehideQuickbusiness logic.frontend/builder/builder-app.tsx (1)
7-48: Dev badge and header tweaks look goodImporting
isDevand conditionally rendering the dev badge in the drag-region header is consistent with existing patterns and side‑effect free. No functional issues spotted.pkg/wshrpc/wshclient/wshclient.go (1)
661-665: StopBuilderCommand wrapper matches existing RPC patternsThe new
StopBuilderCommandhelper mirrors other simplesendRpcRequestCallHelper[any]wrappers and aligns with the TypeScriptStopBuilderCommandsignature.emain/emain-builder.ts (2)
14-18: Adding builderAppId to BuilderWindowType is consistent with new appId flowStoring
builderAppIddirectly on theBrowserWindowsubtype gives main‑process IPC a clear source of truth and aligns with the RT‑info based propagation.
85-88: Window‑local builderAppId assignment matches main RT‑info stateAssigning
typedBuilderWindow.builderAppId = appIdright after creation keeps the Electron side in sync with the"builder:appid"RT‑info key and the renderer’satoms.builderAppId.frontend/app/store/wshclientapi.ts (1)
545-553: StopBuilderCommand TS API is consistent with other RPC helpersThe new
StopBuilderCommandmethod correctly delegates towshRpcCall("stopbuilder", data, opts)and mirrors the existingStartBuilderCommandpattern.frontend/wave.ts (1)
260-274: Builder appId initialization from RT info looks correctReading
"builder:appid"fromGetRTInfoCommandand deriving bothdocument.titleandatoms.builderAppIdfrom that single source cleans up the previous dual‑source behaviour. The try/catch fallback to a neutral title on RT‑info failure is reasonable.frontend/builder/store/builder-apppanel-model.ts (3)
8-34: Secrets tab and model wiring are alignedExtending
TabTypewith"secrets"and addinghasSecretsAtominto the panel model matches the new Secrets tab UX and gives a clean, atom‑backed flag for whether the manifest declares secrets.
81-98: Status subscription correctly updates secrets latchHooking
updateSecretsLatch(status)both in the event handler and after the initialGetBuilderStatusCommandensureshasSecretsAtomis populated as soon as a manifest withsecretsarrives. Since this is a latch, it’s fine that it only flips from false → true.
116-122: updateSecretsLatch implementation matches latch semanticsGuarding on
status?.manifest?.secretsand only settinghasSecretsAtomwhen the secrets map is non‑empty is simple and avoids unnecessary writes. Behaviour is monotonic, as implied by the “latch” name.tsunami/templates/app-init.go.tmpl (1)
1-7: AppInit registration template looks consistent with new AppInitFn APIUsing an
initfunction to callapp.RegisterAppInitFn(AppInit)ensures the app’sAppInithook is wired automatically; just make sure generated/userAppInituses thefunc() errorsignature expected byRegisterAppInitFn.emain/preload.ts (1)
68-71: doRefresh IPC exposure aligns with backend handlerThe new
doRefreshAPI cleanly forwards to the"do-refresh"IPC channel and matches the handler added inemain-ipc.ts; just ensure theElectronApitype incustom.d.tsincludes this method.frontend/app/modals/modalregistry.tsx (1)
8-22: SetSecretDialog modal registration matches existing patternImporting
SetSecretDialogand registering it underSetSecretDialog.displayName || "SetSecretDialog"is consistent with the other entries and should make the modal available viamodalsModel.emain/emain-ipc.ts (1)
29-40: Builder window appId tracking and refresh IPC look correct
openBuilderWindownow de-duplicates bybuilderAppId, which matches the new field onBuilderWindowTypeand avoids multiple windows per appId (with""covering the generic builder)."set-builder-window-appid"updatesbw.builderAppId, keeping the in-memory index consistent with the RT info written fromcreateBuilderWindow."do-refresh"simply callsevent.sender.reloadIgnoringCache(), which is an appropriate per‑webContents refresh hook for the new preload bridge.Also applies to: 424-431, 455-457
tsunami/engine/clientimpl.go (1)
76-96: AppInitFn lifecycle and manifest markers look well-integrated
- Storing an
AppInitFn func() erroronClientImpland invoking it at the start ofrunMainEwith error propagation cleanly integrates initialization into the server lifecycle.RegisterAppInitFnis a simple hook that exposes this to callers without extra complexity.- Wrapping
PrintAppManifestoutput in<AppManifest>/</AppManifest>markers should make downstream parsing more robust when other stdout is present; just ensure any existing consumers of the plain JSON are updated accordingly.Also applies to: 182-199, 514-524
tsunami/app/defaultclient.go (1)
52-57: AppInit registration and static FS helpers are wired sensibly
RegisterAppInitFn(fn func() error)cleanly delegates to the engine’s client and matches the updatedClientImplAPI.ReadStaticFile/OpenStaticFilenow return clear errors whenStaticFSis nil and when the path does not start with"static/", and the guidance to use AppInit for initialization‑time access is helpful for callers.Also applies to: 206-236
tsunami/build/build-ast.go (2)
18-41:buildImportsMapbehavior looks solid; note shallow scan and ignored parse errorsThe import collection logic is straightforward and safe: it only inspects
*.gofiles in the given directory, trims quotes from import paths, and skips files that failparser.ParseFilewithImportsOnly. Just calling out that this is a shallow scan (no subdirs) and treats parse failures as “no imports from that file”, which seems fine for a heuristic/import-check use case but is worth keeping in mind if it’s later used for stricter validation.
47-94: AppInit/init validation is consistent with the new AppInit contract
parseAndValidateAppFile:
- Gracefully handles missing/unparsable
app.goby returningHasAppInit: falseand no error, relying on other checks to enforce file presence.- Explicitly rejects a spec-compliant
init()(no params/results), steering users toAppInitinstead.- Enforces
AppInitto befunc AppInit() errorexactly (no params, exactly one result of typeerror).This matches the described behavior for the new AppInit contract and should fail fast with clear messages when signatures are wrong. No issues from a correctness standpoint.
tsunami/build/build.go (3)
450-476:MainAppFileNamerefactor improves consistencySwitching hard-coded
"app.go"uses inverifyAppPathFsandGetAppModTimeto theMainAppFileNameconstant is a nice cleanup and keeps the main app filename centralized. No behavioral issues here.
598-626: AppInit validation wiring into scaffold copy looks correct
TsunamiBuildInternalnow calls:appInfo, err := parseAndValidateAppFile(appFS) ... scaffoldCount, err := copyScaffoldFS(scaffoldFS, tempDir, appInfo.HasAppInit, opts.Verbose, oc)This cleanly feeds the AST-derived
HasAppInitflag into scaffold copying, socopyScaffoldFScan decide whether to copyapp-init.go.tmpl→app-init.go. Failure to parse/validateapp.goyieldsHasAppInit=falsebut still lets the build proceed (with Go catching syntax issues later), while invalidinit/AppInitsignatures cause immediate errors. The overall flow is consistent and there are no obvious nil or error-handling pitfalls here.
806-822: Manifest delimiter extraction is correct; consider diagnostics if delimiters missing
generateManifestnow extracts only the content between<AppManifest>and</AppManifest>:startIdx := strings.Index(manifestStr, startTag) endIdx := strings.Index(manifestStr, endTag) if startIdx == -1 || endIdx == -1 { return fmt.Errorf("manifest delimiters not found in output") } manifestJSON := manifestStr[startIdx+len(startTag) : endIdx] manifestJSON = strings.TrimSpace(manifestJSON)Semantics are sound: it fails fast when delimiters are absent and writes only the trimmed JSON region. If you run into debugging issues later, you might consider logging a small prefix of
manifestStrtoocon delimiter failures, but as-is this is functionally correct.
| import { ContextMenuModel } from "@/app/store/contextmenu"; | ||
| import { modalsModel } from "@/app/store/modalmodel"; |
There was a problem hiding this comment.
ContextMenuItem type likely needs an explicit import
handleKebabClick declares const menu: ContextMenuItem[] = [...], but this file only imports ContextMenuModel from "@/app/store/contextmenu". Unless ContextMenuItem is declared globally, this will fail type‑checking; consider importing it as a type from the same module, e.g.:
-import { ContextMenuModel } from "@/app/store/contextmenu";
+import { ContextMenuModel, type ContextMenuItem } from "@/app/store/contextmenu";Also applies to: 259-277
🤖 Prompt for AI Agents
In frontend/builder/builder-apppanel.tsx around lines 5-6 (and also where menu
is created around 259-277), the code uses ContextMenuItem[] but only imports
ContextMenuModel, causing TypeScript to fail type-checking; add an explicit
import for the ContextMenuItem type from "@/app/store/contextmenu" (e.g., import
type { ContextMenuItem } from "@/app/store/contextmenu") and update any other
usages to reference that imported type so the menu declaration and related code
compile correctly.
| async restartBuilder() { | ||
| return this.restartBuilder(); | ||
| } |
There was a problem hiding this comment.
restartBuilder is infinite recursion and will crash
restartBuilder currently calls return this.restartBuilder();, which recurses indefinitely and will blow the stack as soon as this.debouncedRestart() fires.
You likely meant to call the backend RestartBuilderAndWaitCommand here. A minimal fix could look like:
- async restartBuilder() {
- return this.restartBuilder();
- }
+ async restartBuilder() {
+ const builderId = globalStore.get(atoms.builderId);
+ try {
+ await RpcApi.RestartBuilderAndWaitCommand(TabRpcClient, { builderid: builderId });
+ } catch (err) {
+ console.error("Failed to restart builder:", err);
+ globalStore.set(this.errorAtom, `Failed to restart builder: ${err.message || "Unknown error"}`);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/builder/store/builder-apppanel-model.ts around lines 210 to 212, the
async method restartBuilder currently calls itself (return
this.restartBuilder()), causing infinite recursion; replace that self-call with
the intended backend invocation (call the RestartBuilderAndWaitCommand via your
RPC/command client or service), await its result, handle errors and return the
appropriate response/void. Ensure you do not re-enter this method and that any
debouncedRestart trigger calls this corrected method.
| const allRequiredBound = | ||
| sortedSecretEntries.filter(([_, meta]) => !meta.optional).every(([name]) => localBindings[name]?.trim()) || | ||
| false; | ||
|
|
||
| return ( | ||
| <div className="w-full h-full flex flex-col p-4"> | ||
| <div className="flex items-center justify-between mb-2"> | ||
| <h2 className="text-lg font-semibold">Secret Bindings</h2> | ||
| <button | ||
| className="px-3 py-1 text-sm font-medium rounded bg-accent/80 text-primary hover:bg-accent transition-colors cursor-pointer disabled:opacity-50 disabled:cursor-not-allowed" | ||
| onClick={handleSave} | ||
| disabled={!isDirty || isSaving} | ||
| > | ||
| {isSaving ? "Saving..." : "Save"} | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="mb-4 p-2 bg-blue-500/10 border border-blue-500/30 rounded text-sm text-secondary"> | ||
| Map app secrets to Wave secret store names. Required secrets must be bound before the app can run | ||
| successfully. | ||
| </div> | ||
|
|
||
| {!allRequiredBound && ( | ||
| <div className="mb-4 p-2 bg-yellow-500/10 border border-yellow-500/30 rounded text-sm text-yellow-600"> | ||
| Some required secrets are not bound yet. | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Treat required secrets as “bound” only when mapped to an existing secret
allRequiredBound currently treats any non-empty binding as satisfying the requirement:
const allRequiredBound =
sortedSecretEntries
.filter(([_, meta]) => !meta.optional)
.every(([name]) => localBindings[name]?.trim()) || false;This ignores whether the binding points to a real secret in availableSecrets, so a required secret bound to a non-existent name will still suppress the “Some required secrets are not bound yet” warning.
You likely want to incorporate validity here, e.g. reuse the same logic as SecretRow:
const allRequiredBound = sortedSecretEntries
.filter(([_, meta]) => !meta.optional)
.every(([name]) => {
const binding = localBindings[name]?.trim();
return binding && availableSecrets.includes(binding);
});That keeps the banner aligned with what will actually work at runtime.
🤖 Prompt for AI Agents
In frontend/builder/tabs/builder-secrettab.tsx around lines 237 to 263, the
allRequiredBound calculation currently treats any non-empty string as a valid
binding; change it to only consider a required secret "bound" when the trimmed
binding exists and matches an entry in availableSecrets. Update the .every check
to trim the binding, ensure it's non-empty, and verify
availableSecrets.includes(binding) (preserving the false fallback if needed) so
the warning banner reflects actual usable secret mappings.
| // ListStaticFiles returns FileInfo for all files in the embedded static filesystem. | ||
| // The Name() of each FileInfo will be the full path prefixed with "static/" (e.g., "static/config.json"), | ||
| // which can be passed directly to ReadStaticFile or OpenStaticFile. | ||
| // Returns an empty slice if StaticFS is nil or on error. | ||
| func ListStaticFiles() ([]fs.FileInfo, error) { | ||
| client := engine.GetDefaultClient() | ||
| if client.StaticFS == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var fileInfos []fs.FileInfo | ||
| err := fs.WalkDir(client.StaticFS, ".", func(path string, d fs.DirEntry, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !d.IsDir() { | ||
| info, err := d.Info() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fullPath := "static/" + path | ||
| fileInfos = append(fileInfos, &staticFileInfo{ | ||
| fullPath: fullPath, | ||
| info: info, | ||
| }) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return fileInfos, nil | ||
| } |
There was a problem hiding this comment.
Align ListStaticFiles behavior with its docstring
The comment says “Returns an empty slice if StaticFS is nil or on error”, but the implementation:
- returns
(nil, nil)whenStaticFSis nil, and - propagates
errfromfs.WalkDirinstead of returning an empty slice.
To avoid surprises, either update the comment to describe the current behavior, or adjust the implementation (e.g., return []fs.FileInfo{} when StaticFS is nil and decide explicitly whether WalkDir errors should be surfaced or converted to an empty result).
🤖 Prompt for AI Agents
In tsunami/app/defaultclient.go around lines 238 to 270, the docstring promises
"Returns an empty slice if StaticFS is nil or on error" but the code returns
(nil, nil) when StaticFS is nil and propagates WalkDir errors; change the
implementation to return an empty slice (e.g., []fs.FileInfo{}) and nil error
when client.StaticFS is nil, and if fs.WalkDir returns an error convert that
into returning an empty slice and nil error (instead of propagating the error)
so the runtime behavior matches the docstring.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/builder/store/builder-apppanel-model.ts (2)
81-98: EnsurehasSecretsAtomis cleared when no secrets are present
updateSecretsLatchonly ever setshasSecretsAtomtotrue(when secrets exist) and does nothing whenstatus.manifest.secretsis missing or empty. This means oncehasSecretsAtombecomestrue, it will never be reset tofalsewithin the same JS lifetime, even if a later manifest has no secrets (e.g., after switching apps without a full reload or when secrets are removed).Consider explicitly setting the atom based on the current status, e.g.:
- updateSecretsLatch(status: BuilderStatusData) { - if (!status?.manifest?.secrets) return; - const secrets = status.manifest.secrets; - if (Object.keys(secrets).length > 0) { - globalStore.set(this.hasSecretsAtom, true); - } - } + updateSecretsLatch(status: BuilderStatusData) { + const secrets = status?.manifest?.secrets; + const hasSecrets = !!secrets && Object.keys(secrets).length > 0; + globalStore.set(this.hasSecretsAtom, hasSecrets); + }This keeps the secrets tab state in sync with whatever the latest manifest reports.
Also applies to: 116-122
215-231: Consider replacing fixed sleeps inswitchBuilderAppwith event/ack-based coordinationThe
setTimeout-style waits (500 ms and 100 ms) aroundStopBuilderCommand,SetRTInfoCommand, anddoRefresh()can be brittle: they add fixed latency and may still race on slower systems or future changes to the stop semantics.If
StopBuilderCommandalready waits for the builder to fully stop, the 500 ms delay might be unnecessary. Otherwise, it would be more robust to key off a builderstatus event (or another explicit ack) rather than a hardcoded delay, and similarly avoid the 100 ms sleep beforedoRefresh()if possible.Not a blocker, but worth tightening when you next touch this flow.
tsunami/app/defaultclient.go (3)
6-16: Static file helpers: consistent behavior and clear errors, with minor DRY opportunityThe new
ReadStaticFile/OpenStaticFileguards and error messages look solid: they fail fast whenStaticFSis nil and clearly enforce the"static/"prefix.You repeat the same “static files not available before app initialization…” error string three times; if callers might want to detect this condition, consider promoting it to a shared
var ErrStaticFilesUnavailable = errors.New(...)(or at least a shared helper) so checks and future updates stay consistent.Also applies to: 206-220, 222-236
28-38:staticFileInfo+ListStaticFilesdesign looks correct; note the non-standardName()The
staticFileInfowrapper andListStaticFilesimplementation look correct:fs.WalkDircovers the tree, skips dirs, and you wrap entries soName()returns"static/"+pathas documented, which is convenient for feeding intoReadStaticFile/OpenStaticFile.One nuance: returning the full path from
Name()is intentionally non-standard forfs.FileInfo(which usually exposes only the base name). Since this API is new and the comment already calls this out, that’s acceptable, but it’d be good to keep this in mind when adding future callers so they don’t assumeName()is just the basename.Also applies to: 238-269
52-56: Document what happens whenRegisterAppInitFnreturns an error
RegisterAppInitFn(fn func() error)is a nice upgrade, but the comment still talks about a “setup function” without explaining how a non-nilerror is treated (e.g., abort app start, logged and ignored, etc.).Consider extending the docstring to spell out the error semantics so app authors know whether returning an error will actually prevent startup or just surface as a logged warning.
tsunami/build/build.go (1)
1135-1149: Consider renaming shadoweddestPathvariable.The variable
destPathis declared twice (lines 1136 and 1144). While valid Go, this shadowing could reduce clarity.Consider using distinct names:
- // Always copy app-main.go.tmpl => app-main.go - destPath := filepath.Join(destDir, "app-main.go") - if err := CopyFileFromFS(scaffoldFS, "app-main.go.tmpl", destPath); err != nil { + // Always copy app-main.go.tmpl => app-main.go + mainDestPath := filepath.Join(destDir, "app-main.go") + if err := CopyFileFromFS(scaffoldFS, "app-main.go.tmpl", mainDestPath); err != nil { return 0, fmt.Errorf("failed to copy app-main.go.tmpl: %w", err) } fileCount++ // Conditionally copy app-init.go.tmpl => app-init.go if hasAppInit { - destPath := filepath.Join(destDir, "app-init.go") - if err := CopyFileFromFS(scaffoldFS, "app-init.go.tmpl", destPath); err != nil { + initDestPath := filepath.Join(destDir, "app-init.go") + if err := CopyFileFromFS(scaffoldFS, "app-init.go.tmpl", initDestPath); err != nil { return 0, fmt.Errorf("failed to copy app-init.go.tmpl: %w", err) } fileCount++ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/builder/store/builder-apppanel-model.ts(5 hunks)pkg/buildercontroller/buildercontroller.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)tsunami/app/defaultclient.go(5 hunks)tsunami/build/build.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/wshrpc/wshserver/wshserver.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 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/builder/store/builder-apppanel-model.ts
🧬 Code graph analysis (4)
pkg/buildercontroller/buildercontroller.go (1)
pkg/waveappstore/waveappstore.go (1)
BuildAppSecretEnv(804-842)
frontend/builder/store/builder-apppanel-model.ts (4)
frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)pkg/wshrpc/wshrpctypes.go (1)
BuilderStatusData(1077-1086)frontend/app/store/global.ts (3)
atoms(816-816)WOS(866-866)getApi(826-826)
tsunami/app/defaultclient.go (1)
tsunami/engine/clientimpl.go (1)
GetDefaultClient(116-118)
tsunami/build/build.go (1)
tsunami/build/buildutil.go (1)
CopyFileFromFS(153-188)
⏰ 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). (4)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (8)
pkg/buildercontroller/buildercontroller.go (2)
309-309: LGTM! Error tagging enables better UX for secret failures.The
(ERR-SECRET)suffix allows the UI to identify secret-related errors and switch to the Secrets tab, which aligns with the PR objectives for improved error handling.Also applies to: 314-314
91-95: No action required—code is safe.Verification confirms the usage is properly protected:
- The only caller (
StopBuilderCommand) checks for nil immediately afterGetControllerand callsStop()in the same scope, avoiding holding references across operationsStop()is idempotent: subsequent calls seebc.process == niland return early (line 486-487)stopProcess_nolock()setsbc.process = nilafter cleanup (line 498), making concurrent calls safe- All stop operations are serialized by
bc.lockfrontend/builder/store/builder-apppanel-model.ts (2)
8-34: Secrets tab type and base state wiring look consistentExtending
TabTypewith"secrets"and introducinghasSecretsAtomfit cleanly with the existing atom pattern and tab handling; no issues from a typing or state-ownership perspective.
210-213:restartBuilderfix (delegation tostartBuilder) looks goodHaving
restartBuilderdelegate tostartBuilder(which already wrapsStartBuilderCommandin a try/catch and handles errors) removes the previous recursion issue and keeps restart semantics centralized.tsunami/build/build.go (4)
33-33: Good practice: Centralized filename constant.Introducing
MainAppFileNameeliminates hardcoded "app.go" strings throughout the codebase, improving maintainability.
491-498: Past review issue resolved.The copy-paste error mentioned in the previous review has been correctly fixed. The code now properly validates both
app-main.go.tmplandapp-init.go.tmplwith appropriate error messages.
1072-1173: Well-structured conditional scaffold copying.The refactored
copyScaffoldFSproperly handles the newhasAppInitparameter, always copying the main template while conditionally copying the init template. The use ofslices.Containsto avoid duplicate package file copies is a clean solution.
599-602: No issues found. Function exists and is properly accessible.The
parseAndValidateAppFilefunction is defined inbuild-ast.go(line 47) within the same package and is correctly called inbuild.go. The function signature matches the call site, and it handles all edge cases appropriately: missing files return a safe default, parse errors return a safe default, and invalid app initialization functions return descriptive errors.
| // Extract manifest between delimiters | ||
| manifestStr := string(manifestOutput) | ||
| startTag := "<AppManifest>" | ||
| endTag := "</AppManifest>" | ||
| startIdx := strings.Index(manifestStr, startTag) | ||
| endIdx := strings.Index(manifestStr, endTag) | ||
|
|
||
| if startIdx == -1 || endIdx == -1 { | ||
| return fmt.Errorf("manifest delimiters not found in output") | ||
| } | ||
|
|
||
| manifestJSON := manifestStr[startIdx+len(startTag) : endIdx] | ||
| manifestJSON = strings.TrimSpace(manifestJSON) | ||
|
|
||
| manifestPath := filepath.Join(tempDir, "manifest.json") | ||
| if err := os.WriteFile(manifestPath, manifestOutput, 0644); err != nil { | ||
| if err := os.WriteFile(manifestPath, []byte(manifestJSON), 0644); err != nil { | ||
| return fmt.Errorf("failed to write manifest.json: %w", err) | ||
| } |
There was a problem hiding this comment.
Add bounds validation for manifest delimiter extraction.
The code checks if delimiter indices are -1 but doesn't validate that endIdx > startIdx. If the delimiters appear in reverse order or overlap, the slice operation on Line 818 could panic or extract incorrect content.
Apply this diff to add bounds validation:
if startIdx == -1 || endIdx == -1 {
return fmt.Errorf("manifest delimiters not found in output")
}
+ if endIdx <= startIdx {
+ return fmt.Errorf("manifest delimiters are malformed (end tag before start tag)")
+ }
manifestJSON := manifestStr[startIdx+len(startTag) : endIdx]📝 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.
| // Extract manifest between delimiters | |
| manifestStr := string(manifestOutput) | |
| startTag := "<AppManifest>" | |
| endTag := "</AppManifest>" | |
| startIdx := strings.Index(manifestStr, startTag) | |
| endIdx := strings.Index(manifestStr, endTag) | |
| if startIdx == -1 || endIdx == -1 { | |
| return fmt.Errorf("manifest delimiters not found in output") | |
| } | |
| manifestJSON := manifestStr[startIdx+len(startTag) : endIdx] | |
| manifestJSON = strings.TrimSpace(manifestJSON) | |
| manifestPath := filepath.Join(tempDir, "manifest.json") | |
| if err := os.WriteFile(manifestPath, manifestOutput, 0644); err != nil { | |
| if err := os.WriteFile(manifestPath, []byte(manifestJSON), 0644); err != nil { | |
| return fmt.Errorf("failed to write manifest.json: %w", err) | |
| } | |
| // Extract manifest between delimiters | |
| manifestStr := string(manifestOutput) | |
| startTag := "<AppManifest>" | |
| endTag := "</AppManifest>" | |
| startIdx := strings.Index(manifestStr, startTag) | |
| endIdx := strings.Index(manifestStr, endTag) | |
| if startIdx == -1 || endIdx == -1 { | |
| return fmt.Errorf("manifest delimiters not found in output") | |
| } | |
| if endIdx <= startIdx { | |
| return fmt.Errorf("manifest delimiters are malformed (end tag before start tag)") | |
| } | |
| manifestJSON := manifestStr[startIdx+len(startTag) : endIdx] | |
| manifestJSON = strings.TrimSpace(manifestJSON) | |
| manifestPath := filepath.Join(tempDir, "manifest.json") | |
| if err := os.WriteFile(manifestPath, []byte(manifestJSON), 0644); err != nil { | |
| return fmt.Errorf("failed to write manifest.json: %w", err) | |
| } |
🤖 Prompt for AI Agents
In tsunami/build/build.go around lines 807 to 824, the code checks for missing
delimiters but doesn't validate that the end delimiter comes after the start
delimiter, which can cause an invalid slice or wrong content; update the logic
to compute contentStart := startIdx + len(startTag) and then verify endIdx >
contentStart (or at minimum endIdx > startIdx) and return a descriptive error if
not, before slicing and writing manifest.json.
