add tsunami view in wave #2350
add tsunami view in wave #2350
Conversation
…plement the tsunami controller into...
…ltiple conns), update tw css...
|
Caution Review failedThe pull request is closed. WalkthroughThe PR introduces a pluggable Controller interface and per-block controller registry, replacing the previous BlockController implementation, and adds ShellController and TsunamiController implementations with public lifecycle, status and input APIs (including a tsunami port in runtime status). It extends tsunami tooling (AppPath, NodePath, .tsapp ZIP FS support, packaging, build cache, tailwind invocation, templates) and wires tsunami into builds. Frontend additions include a Tsunami view/model, block registration, new meta/settings keys/types, per-connection SSE channels, clientid URL priority, expanded VDom tags, and Vite watch ignores. Supporting utilities added: ReaderLineBuffer, DirFS/path-to-FS, wavebase electron-path env caching, taskfile scaffold tasks, and various file/template updates. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tsunami/engine/clientimpl.go (1)
166-217: Server shutdown not tied to lifecycle; leaks on DoneCh.
listenAndServewaits onctx.Done(), butrunMainEpassescontext.Background(). Tie shutdown toDoneChto avoid orphaned server goroutine.Apply:
- go func() { - <-ctx.Done() - log.Printf("Context canceled, shutting down server...") - if err := server.Shutdown(context.Background()); err != nil { - log.Printf("Server shutdown error: %v", err) - } - }() + go func() { + select { + case <-ctx.Done(): + case <-c.DoneCh: + } + log.Printf("Shutting down server...") + if err := server.Shutdown(context.Background()); err != nil { + log.Printf("Server shutdown error: %v", err) + } + }()tsunami/engine/serverhandlers.go (1)
376-384: Bug: closed channel causes tight loop; break only exits select.When
eventChis closed,eventis zero-valued and this path spins. Use the comma-ok receive and return (or labeled break).Apply:
- case event := <-eventCh: - if event.Event == "" { - break - } + case event, ok := <-eventCh: + if !ok { + return + } + if event.Event == "" { + continue + } fmt.Fprintf(w, "event: %s\n", event.Event) fmt.Fprintf(w, "data: %s\n", string(event.Data)) fmt.Fprintf(w, "\n") rc.Flush()go.mod (1)
3-3: Invalid go directive format (patch version).
go.mod’sgodirective should not include a patch number. Usego 1.24and, if you need an exact toolchain, add a separatetoolchaindirective.Apply:
-go 1.24.6 +go 1.24 +toolchain go1.24.6Taskfile.yml (1)
509-515: Use cross‑platform copy helper instead of raw cp.Direct
cpcalls break on Windows. Reuse the existingcopyfiles:*:*task for portability.Apply:
- - cp ../templates/package.json.tmpl scaffold/package.json - - cd scaffold && npm install - - cp -r dist scaffold/ - - cp ../templates/app-main.go.tmpl scaffold/app-main.go - - cp ../templates/tailwind.css scaffold/ - - cp ../templates/gitignore.tmpl scaffold/.gitignore + - task: copyfiles:'../templates/package.json.tmpl':'scaffold/package.json' + - cd scaffold && npm install + - task: copyfiles:'dist':'scaffold' + - task: copyfiles:'../templates/app-main.go.tmpl':'scaffold/app-main.go' + - task: copyfiles:'../templates/tailwind.css':'scaffold/tailwind.css' + - task: copyfiles:'../templates/gitignore.tmpl':'scaffold/.gitignore'tsunami/build/build.go (2)
271-283: Use discovered Go executable instead of assuming PATHverifyEnvironment may succeed via common install paths, but later you invoke “go” via PATH, causing failures.
- tidyCmd := exec.Command("go", "mod", "tidy") + goPath, err := findGoExecutable() + if err != nil { + return fmt.Errorf("go not found for mod tidy: %w", err) + } + tidyCmd := exec.Command(goPath, "mod", "tidy")
645-648: Same issue in go build path usageReuse the resolved go binary.
- buildCmd := exec.Command("go", args...) + goPath, err := findGoExecutable() + if err != nil { + return fmt.Errorf("go not found for build: %w", err) + } + buildCmd := exec.Command(goPath, args...)
🧹 Nitpick comments (27)
tsunami/engine/clientimpl.go (2)
219-226: Channel capacity and duplicate registration handling.
- Extract channel capacity 100 to a const for visibility/tuning.
- If a duplicate connectionId arrives, consider closing/overwriting the old channel deterministically and logging once.
Apply:
+const sseChanCap = 100 ... - ch := make(chan ssEvent, 100) + ch := make(chan ssEvent, sseChanCap) + if old, ok := c.SSEChannels[connectionId]; ok { + close(old) + }
228-236: Safe close is fine; minor nit.Close+delete under the same lock is correct. Optionally return bool to signal whether an entry existed (useful for callers/tests).
tsunami/engine/serverhandlers.go (1)
343-349: Per-connection channel setup LGTM; consider stronger ID.Timestamp-based IDs are fine; a UUID (
uuid.NewString()) avoids edge collisions and makes logs clearer.emain/emain-util.ts (1)
65-71: Optional: de‑duplicate the dev index allowlist.Consider centralizing the dev URLs (parse once and compare origins) to avoid future drift when ports change.
tsunami/frontend/src/util/clientid.ts (3)
6-10: Update doc to reflect URL override precedenceThe comment omits the new “clientid” query behavior. Clarify precedence and persistence.
Apply this diff:
- * Gets or creates a unique client ID for this browser tab/window. - * The client ID is stored in sessionStorage and persists for the lifetime of the tab. - * If no client ID exists, a new UUID is generated and stored. + * Gets or creates a unique client ID. + * Precedence: + * 1) If the URL has ?clientid=..., that value is used (and not persisted unless explicitly stored). + * 2) Otherwise, a sessionStorage-scoped UUID is used/created for this tab.
12-19: Option: persist URL override to sessionStorage for stability across SPA route changesIf the query param can disappear during client-side routing, persist the validated value to avoid client ID churn.
Apply on top of the validation diff:
- if (urlClientId && isValidUuid(urlClientId)) { - return urlClientId; - } + if (urlClientId && isValidUuid(urlClientId)) { + sessionStorage.setItem(CLIENT_ID_KEY, urlClientId); + return urlClientId; + }
32-34: Clarify/extend clear behavior when URL override is present
clearClientId()only clears storage; a URLclientidwill still take precedence. Either document this or provide a helper to remove it from the URL.Apply this addition (new helper, no breaking changes):
export function clearClientIdFromUrl(): void { const url = new URL(window.location.href); url.searchParams.delete("clientid"); history.replaceState(null, "", url.toString()); }tsunami/frontend/package.json (1)
30-35: Tailwind bump aligns with templates; looks good.Versions match the scaffold template and should reduce drift.
Optional: add engines/packageManager to stabilize local setups.
Example:"private": true, "type": "module", + "engines": { "node": ">=18.18" }, + "packageManager": "npm@10"go.mod (1)
117-118: Leaving a local replace in root go.mod may break consumers.The replace to
./tsunamiis great for dev, but external consumers or CI that build the root module may fail if./tsunamiisn’t present. Consider guarding this behind a build profile/workflow step or documenting it, and ensure release automation doesn’t strip it accidentally.tsunami/templates/package.json.tmpl (1)
1-16: Mark scaffold package.json as private to avoid accidental publish.Safer for templates used in local scaffolds.
Apply:
{ "name": "tsunami-scaffold", "version": "1.0.0", + "private": true, "main": "index.js",Optional:
"author": { "name": "Command Line Inc", "email": "[email protected]" }, + "engines": { "node": ">=18.18" }, "dependencies": {tsunami/frontend/src/tailwind.css (1)
64-111: Global overscroll and 6px scrollbars: consider a11y and scoping.
- 6px scrollbars can be hard to use; add a media override for high‑contrast/forced‑colors users and consider 8–10px.
- Global overscroll suppression may hinder expected OS gestures; scope to the app container if possible.
Example additions:
@media (forced-colors: active) { * { scrollbar-color: ButtonText ButtonFace; } } @media (any-pointer: coarse) { ::-webkit-scrollbar { width: 10px !important; height: 10px !important; } }If feasible, scope scrollbar rules to a container (e.g., .app or .tsunami-root) instead of global selectors.
Taskfile.yml (1)
488-500: Two scaffold flows (template vs npm init) risk drift.You now have both a template‑based flow and an npm‑init flow. Consider marking this task internal too or documenting when to use each to avoid divergence.
frontend/types/gotypes.d.ts (1)
594-598: Meta keys for Tsunami config — OK; consider SettingsType if some are globalThe MetaType additions look fine. If any of these are intended as global app settings (vs per-block), consider mirroring under SettingsType for type-safe access in UI.
tsunami/cmd/main-tsunami.go (1)
15-16: CLI/env updates (NodePath optional, AppPath/MoveFileBack) — looks good; minor DX nits
- EnvTsunamiNodePath + optional assignment: OK.
- AppPath rename and MoveFileBack: wiring looks consistent with build.BuildOpts.
- Minor DX: printing errors via fmt.Println writes to stdout; consider stderr for failures.
Apply this small nit if desired:
- fmt.Println(err) + fmt.Fprintln(os.Stderr, err)Also applies to: 51-54, 60-75, 88-103
pkg/blockcontroller/tsunamicontroller.go (2)
351-361: Drop noisy “WAIT RETURN” log or gate behind verbose flagThis log is chatty for normal runs.
- log.Printf("WAIT RETURN: %v\n", tsunamiProc.WaitRtn) + // log.Printf("tsunami wait returned: %v", tsunamiProc.WaitRtn) // consider gating by a verbose flag
260-266: Honor graceful stop before hard killYou pass --close-on-stdin; try closing stdin first and waiting briefly before Process.Kill().
- if c.tsunamiProc.Cmd.Process != nil { - c.tsunamiProc.Cmd.Process.Kill() - } - - if c.tsunamiProc.StdinWriter != nil { - c.tsunamiProc.StdinWriter.Close() - } + if c.tsunamiProc.StdinWriter != nil { + _ = c.tsunamiProc.StdinWriter.Close() + } + if c.tsunamiProc.Cmd.Process != nil { + if graceful { + // Give the app a moment to exit after stdin closes. + done := make(chan struct{}) + go func() { <-c.tsunamiProc.WaitCh; close(done) }() + select { + case <-done: + case <-time.After(2 * time.Second): + c.tsunamiProc.Cmd.Process.Kill() + } + } else { + c.tsunamiProc.Cmd.Process.Kill() + } + }frontend/app/view/tsunami/tsunami.tsx (3)
31-35: Fix atom typing to allow null initial stateAvoid type-asserting null into a non-null atom; use a union type.
- this.shellProcFullStatus = jotai.atom(null) as jotai.PrimitiveAtom<BlockControllerRuntimeStatus>; + this.shellProcFullStatus = jotai.atom<BlockControllerRuntimeStatus | null>(null);
149-151: Consider sandboxing the iframeEven though it’s localhost, sandboxing reduces risk from the app content.
- return <iframe src={iframeUrl} className="w-full h-full border-0" title="Tsunami Application" name={`tsunami:${model.blockId}`} />; + return ( + <iframe + src={iframeUrl} + className="w-full h-full border-0" + title="Tsunami Application" + name={`tsunami:${model.blockId}`} + sandbox="allow-scripts allow-same-origin" + /> + );
51-58: Remove dev console.log or guard itKeep logs behind a dev flag to avoid noisy consoles.
tsunami/build/build.go (1)
790-799: Avoid shadowing the imported url package nameMinor readability nit.
- port := ParseTsunamiPort(line) - if port > 0 { - url := fmt.Sprintf("http://localhost:%d", port) + port := ParseTsunamiPort(line) + if port > 0 { + openURL := fmt.Sprintf("http://localhost:%d", port) if verbose { - log.Printf("Opening browser to %s", url) + log.Printf("Opening browser to %s", openURL) } - go util.OpenBrowser(url, 100*time.Millisecond) + go util.OpenBrowser(openURL, 100*time.Millisecond) browserOpened = truepkg/blockcontroller/shellcontroller.go (2)
357-371: Receiver name inconsistency (bc vs sc)Methods in this type mix receiver names (sc vs bc). Prefer a consistent receiver name for readability.
91-115: Stop(): lock/unlock pattern is correct, but consider documenting the graceful pathYou unlock while waiting on DoneCh and re-lock—balanced by the deferred unlock. Add a short comment to avoid future refactor mistakes.
pkg/blockcontroller/blockcontroller.go (5)
226-233: Runtime status accessor is fine, but consider returning a copy.
To minimize race detector noise, consider returning a copied struct.
292-312: Append eventing looks good; consider size guard/chunking.
Large writes can blow up SSE payloads. Enforce a max append size or chunk beforePublish.I can add a simple chunker honoring
DefaultTermMaxFileSizeif you want.
314-342: Useerrors.Isfor robust not-exist checks; also for the cache delete.
Direct equality may miss wrapped errors.func HandleTruncateBlockFile(blockId string) error { ctx, cancelFn := context.WithTimeout(context.Background(), DefaultTimeout) defer cancelFn() err := filestore.WFS.WriteFile(ctx, blockId, wavebase.BlockFile_Term, nil) - if err == fs.ErrNotExist { + if errors.Is(err, fs.ErrNotExist) { return nil } if err != nil { return fmt.Errorf("error truncating blockfile: %w", err) } err = filestore.WFS.DeleteFile(ctx, blockId, wavebase.BlockFile_Cache) - if err == fs.ErrNotExist { + if errors.Is(err, fs.ErrNotExist) { err = nil }Add import:
import "errors"
344-347: Double logging may be noisy.
blocklogger.Infofpluslog.Printfduplicates entries. Prefer one path or guard behind a debug flag.
46-46: Centralize DefaultGracefulKillWait — remove duplicate declaration.Two definitions found: pkg/shellexec/shellexec.go:36 (used at 57) and pkg/blockcontroller/blockcontroller.go:46; delete the duplicate in blockcontroller and reference the single definition in shellexec (or move the constant to a shared package).
📜 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 (28)
Taskfile.yml(2 hunks)electron.vite.config.ts(1 hunks)emain/emain-util.ts(3 hunks)emain/emain-wavesrv.ts(2 hunks)frontend/app/block/block.tsx(2 hunks)frontend/app/view/tsunami/tsunami.tsx(1 hunks)frontend/types/gotypes.d.ts(2 hunks)go.mod(2 hunks)pkg/blockcontroller/.gitignore(1 hunks)pkg/blockcontroller/blockcontroller.go(3 hunks)pkg/blockcontroller/shell_controller.go(0 hunks)pkg/blockcontroller/shellcontroller.go(1 hunks)pkg/blockcontroller/tsunamicontroller.go(1 hunks)pkg/service/blockservice/blockservice.go(1 hunks)pkg/utilds/readerlinebuffer.go(1 hunks)pkg/wavebase/wavebase.go(4 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)tsunami/app/defaultclient.go(2 hunks)tsunami/build/build.go(20 hunks)tsunami/cmd/main-tsunami.go(3 hunks)tsunami/engine/clientimpl.go(3 hunks)tsunami/engine/serverhandlers.go(2 hunks)tsunami/frontend/package.json(1 hunks)tsunami/frontend/src/tailwind.css(1 hunks)tsunami/frontend/src/util/clientid.ts(1 hunks)tsunami/templates/package.json.tmpl(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/blockcontroller/shell_controller.go
🧰 Additional context used
🧬 Code graph analysis (12)
frontend/app/block/block.tsx (1)
frontend/app/view/tsunami/tsunami.tsx (1)
TsunamiViewModel(175-175)
pkg/service/blockservice/blockservice.go (1)
pkg/blockcontroller/blockcontroller.go (1)
GetBlockControllerRuntimeStatus(226-232)
pkg/blockcontroller/shellcontroller.go (10)
pkg/waveobj/wtype.go (3)
RuntimeOpts(264-267)TermSize(304-307)OType_Block(30-30)pkg/shellexec/shellexec.go (2)
ShellProc(48-54)CommandOptsType(38-46)pkg/blockcontroller/blockcontroller.go (13)
BlockInputUnion(48-52)Controller(64-69)Status_Init(36-36)Status_Done(35-35)BlockControllerRuntimeStatus(54-61)DefaultTimeout(45-45)HandleAppendBlockFile(292-312)BlockController_Shell(28-28)BlockController_Cmd(29-29)HandleTruncateBlockFile(314-342)DefaultTermMaxFileSize(40-40)Status_Running(34-34)MaxInitScriptSize(42-42)pkg/wstore/wstore_dbops.go (6)
DBMustGet(138-149)DBFindTabForBlockId(341-368)DBFindWorkspaceForTabId(370-394)DBGetSingleton(102-105)DBGet(133-136)DBUpdate(303-320)pkg/wps/wpstypes.go (2)
WaveEvent(21-27)Event_ControllerStatus(12-12)pkg/wavebase/wavebase.go (3)
BlockFile_Term(42-42)GetDomainSocketName(170-172)BlockFile_Env(45-45)pkg/wshutil/wshutil.go (3)
WaveOSCPrefix(39-39)EncodeWaveOSCBytes(77-116)WaveServerOSC(36-36)pkg/wshutil/wshproxy.go (1)
MakeRpcProxy(26-32)pkg/wshutil/wshcmdreader.go (1)
MakePtyBuffer(34-48)pkg/util/shellutil/tokenswap.go (1)
TokenSwapEntry(19-26)
frontend/app/view/tsunami/tsunami.tsx (6)
pkg/blockcontroller/blockcontroller.go (1)
BlockControllerRuntimeStatus(54-61)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/global.ts (2)
WOS(809-809)atoms(762-762)frontend/app/store/wps.ts (1)
waveEventSubscribe(146-146)frontend/app/store/wshclientapi.ts (1)
RpcApi(492-492)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(152-152)
tsunami/app/defaultclient.go (1)
pkg/util/packetparser/packetparser.go (1)
Parse(47-71)
emain/emain-wavesrv.ts (1)
emain/emain-util.ts (2)
WaveAppElectronExecPath(8-8)getElectronExecPath(10-12)
pkg/wshrpc/wshserver/wshserver.go (1)
pkg/blockcontroller/blockcontroller.go (2)
StopBlockController(234-240)SendInput(250-256)
tsunami/cmd/main-tsunami.go (1)
tsunami/build/build.go (1)
BuildOpts(30-40)
tsunami/build/build.go (1)
tsunami/util/util.go (1)
OpenBrowser(81-102)
tsunami/engine/clientimpl.go (2)
tsunami/engine/rootelem.go (1)
RootElem(36-49)tsunami/app/defaultclient.go (1)
SendAsyncInitiation(42-44)
pkg/blockcontroller/tsunamicontroller.go (7)
pkg/utilds/readerlinebuffer.go (2)
ReaderLineBuffer(9-18)MakeReaderLineBuffer(20-35)pkg/wavebase/wavebase.go (5)
IsDevMode(106-108)GetHomeDir(130-136)TryMkdirs(207-223)ExpandHomeDir(138-152)GetWaveAppElectronExecPath(126-128)tsunami/build/build.go (5)
GetAppModTime(331-338)BuildOpts(30-40)TsunamiBuild(459-467)GetAppName(46-48)ParseTsunamiPort(804-827)pkg/blockcontroller/blockcontroller.go (7)
Status_Running(34-34)Status_Done(35-35)BlockControllerRuntimeStatus(54-61)SendInput(250-256)BlockInputUnion(48-52)Controller(64-69)Status_Init(36-36)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (2)
WaveEvent(21-27)Event_ControllerStatus(12-12)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)
pkg/blockcontroller/blockcontroller.go (10)
pkg/waveobj/wtype.go (4)
TermSize(304-307)RuntimeOpts(264-267)Block(279-287)Block(289-291)pkg/waveobj/metamap.go (1)
MetaMapType(8-8)pkg/wstore/wstore_dbops.go (1)
DBMustGet(138-149)pkg/waveobj/metaconsts.go (2)
MetaKey_Controller(11-11)MetaKey_Connection(19-19)pkg/blockcontroller/shellcontroller.go (2)
ShellController(47-63)MakeShellController(66-75)pkg/blockcontroller/tsunamicontroller.go (2)
TsunamiController(36-46)MakeTsunamiController(419-426)pkg/filestore/blockstore.go (1)
WFS(49-52)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (5)
WaveEvent(21-27)Event_BlockFile(14-14)WSFileEventData(47-52)FileOp_Append(42-42)FileOp_Truncate(43-43)pkg/wavebase/wavebase.go (2)
BlockFile_Term(42-42)BlockFile_Cache(43-43)
⏰ 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: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (23)
tsunami/engine/clientimpl.go (2)
66-73: Init changes LGTM.Good: explicit DoneCh, ServerId, RootElem, and dedicated ServeMux.
259-262: Async initiation delegation LGTM; verify call timing.If this fires before any connection registers, the event is dropped by design. Ensure caller semantics tolerate that.
electron.vite.config.ts (1)
81-81: LGTM: prevent renderer HMR thrash from emain changes.Ignoring "emain/**" in the renderer watch set is appropriate given main/preload have their own pipelines.
emain/emain-util.ts (1)
8-12: Expose Electron exec path via env — LGTM.Trivial and cross‑platform; consumers can now read the path reliably.
emain/emain-wavesrv.ts (1)
10-10: Pass Electron exec path to wavesrv — LGTM; please verify values on each OS.Propagation matches the new Go var. Confirm macOS returns the app binary in production and Windows returns the .exe path as expected.
Also applies to: 62-63
pkg/wavebase/wavebase.go (1)
29-35: Env var plumbing for WAVETERM_ELECTRONEXECPATH — LGTM; ensure consumers handle empty string.Caching/unsetting is consistent with existing vars. If the env isn’t set (older launchers/dev), GetWaveAppElectronExecPath() returns "", so downstream code should treat it as optional.
Also applies to: 50-55, 98-100, 126-128
tsunami/frontend/src/util/clientid.ts (1)
12-19: Do not enforce UUID-only for URL clientid — repository uses non‑UUID client IDs (e.g. "wave:...")Found callers that embed non-UUID values into the URL (frontend/app/view/tsunami/tsunami.tsx uses ?clientid=wave:${model.blockId}); applying the suggested RFC‑4122-only check will break those flows. Choose one:
- Migrate callers to always supply RFC‑4122 UUIDs (update callers: frontend/app/view/tsunami/tsunami.tsx, frontend/wave.ts, tsunami/frontend/src/model/tsunami-model.tsx) and then add the isValidUuid guard in tsunami/frontend/src/util/clientid.ts.
- Or accept URL overrides but validate against an allowlist (RFC‑4122 UUID OR known prefixes like /^wave:/) and — if accepted — persist the override to sessionStorage for SPA navigations (store only after validation).
Also review server-side checks (tsunami/engine/clientimpl.go, serverhandlers.go) to ensure they expect the same formats and do not allow spoofing based solely on a URL param.
Likely an incorrect or invalid review comment.
pkg/blockcontroller/.gitignore (1)
1-1: LGTM on local ignore.Ignoring *.old here is harmless and reduces churn.
pkg/waveobj/metaconsts.go (1)
117-121: New tsunami meta keys: consistent with existing patterns.Naming and section‑clear semantics match other meta keys.
Ensure generators and TS bindings were refreshed so front‑end types include these keys (Taskfile “generate”).
pkg/waveobj/wtypemeta.go (1)
120-124: Struct tags align with section‑clear behavior; LGTM.
tsunami:*will be handled by MergeMeta like other sections.go.mod (1)
36-36: Resolved — module path matches exactly.
tsunami/go.mod (line 1) declaresmodule github.com/wavetermdev/waveterm/tsunami, so the zero pseudo‑version paired with a local replace is correct and should not cause a mismatched module path error.frontend/app/block/block.tsx (1)
15-16: Tsunami block registration looks correctImport and registry wiring for TsunamiViewModel are consistent with existing patterns. No concerns here.
Confirm blockViewToIcon/blockViewToName include a mapping for "tsunami" (or acceptable fallback) so headers/icons render as intended.
Also applies to: 52-53
pkg/wshrpc/wshserver/wshserver.go (2)
303-306: Stop now delegates to central controller API (idempotent) — OKShifting to blockcontroller.StopBlockController(blockId) is cleaner and safe if absent. Looks good.
314-328: Input path now centralizes controller lookup; verify error propagationDelegation to blockcontroller.SendInput(...) removes the prior nil-check. SendInput returns an error when no controller exists; this changes behavior vs. silently no-op. Ensure RPC clients and logs expect/handle that error.
frontend/types/gotypes.d.ts (1)
63-64: Adds tsunamiport to runtime status — OKOptional number aligns with backend addition. Looks good.
pkg/service/blockservice/blockservice.go (1)
33-35: Behavior change: GetControllerStatus now returns (status, nil) — callers must handle nil statusGetControllerStatus now returns the runtime status and a nil error; a nil status indicates "no controller" and must be checked before dereferencing to avoid panics.
File: pkg/service/blockservice/blockservice.go (lines shown)
func (bs *BlockService) GetControllerStatus(ctx context.Context, blockId string) (*blockcontroller.BlockControllerRuntimeStatus, error) { return blockcontroller.GetBlockControllerRuntimeStatus(blockId), nil }
- Repo scan produced no matches for ".GetControllerStatus(" — downstream usage not verified.
- Action: run a repo-wide search for GetControllerStatus(...) and ensure all callers check status == nil before use, or change the API to return a non-nil error when no controller is present.
pkg/blockcontroller/tsunamicontroller.go (1)
216-218: Executable-bit check: consider Windows-only guard retained; on Unix ensure file mode is preservedLooks fine. Verify that go build preserves +x on the cache path; otherwise explicitly chmod 0755 on Unix after moving/copying.
pkg/blockcontroller/shellcontroller.go (1)
486-492: Good: atomic status update and event publish wrapperClean pattern to update state and emit events once.
tsunami/build/build.go (1)
555-557: Confirm @tailwindcss/cli is present in scaffold's node_modules — copyScaffoldSelective only symlinks node_modulescopyScaffoldSelective creates a symlink for "node_modules" into tempDir (tsunami/build/buildutil.go:154–160); build.go requires scaffoldPath/node_modules (tsunami/build/build.go:386–394) and generateAppTailwindCss invokes node with "node_modules/@tailwindcss/cli/dist/index.mjs" (tsunami/build/build.go:675–677). Ensure scaffoldPath/node_modules contains @tailwindcss/cli or modify the copy/validation logic to include/verify it.
pkg/blockcontroller/blockcontroller.go (4)
28-31: New controller type constant looks good.
Good discriminator to gate controller selection logic.
137-153: Controller morphing logic is fine.
Type switch is clear and maintainable.
203-221: Status read semantics — ensure TsunamiController populates ShellProcStatus or gate by controller type.blockcontroller.go (pkg/blockcontroller/blockcontroller.go:203-221) currently reads status.ShellProcStatus for shell/cmd and tsunami. Verify pkg/blockcontroller/tsunamicontroller.go:GetRuntimeStatus (≈line 280) actually sets ShellProcStatus; if it does not, either populate ShellProcStatus there or change the start-condition to only use ShellProcStatus for BlockController_Shell / BlockController_Cmd (and use a tsunami-specific field/check for TsunamiController).
63-69: No change needed — GetRuntimeStatus already returns a locked snapshot.
ShellController.GetRuntimeStatus (pkg/blockcontroller/shellcontroller.go:130) and TsunamiController.GetRuntimeStatus (pkg/blockcontroller/tsunamicontroller.go:280) both take the controller lock and return a newly-created/copy BlockControllerRuntimeStatus, so controller internals are not exposed to concurrent mutation.
| log.Printf("stopping blockcontroller %s due to controller type change\n", blockId) | ||
| StopBlockController(blockId) | ||
| time.Sleep(100 * time.Millisecond) | ||
| deleteController(blockId) | ||
| existing = nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid time.Sleep after Stop; make Stop synchronous or wait for completion.
Fixed sleeps are brittle. Either rely on Stop being blocking, or explicitly wait until status transitions to done.
Minimal change (assumes Stop blocks):
- StopBlockController(blockId)
- time.Sleep(100 * time.Millisecond)
- deleteController(blockId)
+ StopBlockController(blockId)
+ deleteController(blockId)If Stop is async, add a WaitStopped(ctx) or poll with a bounded timeout; I can wire that if desired.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/blockcontroller/blockcontroller.go around lines 155 to 161, avoid the
brittle time.Sleep after Stop: make Stop blocking or wait explicitly for the
controller to reach a stopped/done state. Replace the Sleep by either calling a
blocking Stop (ensure Stop returns only after shutdown) or add/wire a
WaitStopped(ctx, timeout) or poll loop that checks the controller status with a
bounded timeout/context cancellation; after WaitStopped/poll returns success,
call deleteController(blockId) and set existing = nil, otherwise log/handle a
timeout error.
| if !needsReplace && (controllerName == BlockController_Shell || controllerName == BlockController_Cmd) { | ||
| connName := blockData.Meta.GetString(waveobj.MetaKey_Connection, "") | ||
| if existingStatus.ShellProcStatus == Status_Running && existingStatus.ShellProcConnName != connName { | ||
| log.Printf("stopping blockcontroller %s due to conn change\n", blockId) | ||
| StopBlockControllerAndSetStatus(blockId, Status_Init) | ||
| time.Sleep(100 * time.Millisecond) | ||
| // Don't delete, will reuse same controller type | ||
| existing = getController(blockId) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove arbitrary sleep on connection change.
Same reasoning; don’t sleep for 100ms—wait deterministically or rely on synchronous Stop.
- StopBlockControllerAndSetStatus(blockId, Status_Init)
- time.Sleep(100 * time.Millisecond)
- // Don't delete, will reuse same controller type
- existing = getController(blockId)
+ StopBlockControllerAndSetStatus(blockId, Status_Init)
+ // Don't delete, will reuse same controller type
+ existing = getController(blockId)📝 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.
| if !needsReplace && (controllerName == BlockController_Shell || controllerName == BlockController_Cmd) { | |
| connName := blockData.Meta.GetString(waveobj.MetaKey_Connection, "") | |
| if existingStatus.ShellProcStatus == Status_Running && existingStatus.ShellProcConnName != connName { | |
| log.Printf("stopping blockcontroller %s due to conn change\n", blockId) | |
| StopBlockControllerAndSetStatus(blockId, Status_Init) | |
| time.Sleep(100 * time.Millisecond) | |
| // Don't delete, will reuse same controller type | |
| existing = getController(blockId) | |
| } | |
| } | |
| } | |
| if !needsReplace && (controllerName == BlockController_Shell || controllerName == BlockController_Cmd) { | |
| connName := blockData.Meta.GetString(waveobj.MetaKey_Connection, "") | |
| if existingStatus.ShellProcStatus == Status_Running && existingStatus.ShellProcConnName != connName { | |
| log.Printf("stopping blockcontroller %s due to conn change\n", blockId) | |
| StopBlockControllerAndSetStatus(blockId, Status_Init) | |
| // Don't delete, will reuse same controller type | |
| existing = getController(blockId) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/blockcontroller/blockcontroller.go around lines 163 to 173, remove the
arbitrary time.Sleep(100 * time.Millisecond) after calling
StopBlockControllerAndSetStatus; instead, make the stop deterministic by either
(a) changing StopBlockControllerAndSetStatus to block until the controller is
fully stopped (return when stopped) or (b) poll/wait for a clear condition
(e.g., existingStatus or getController(blockId) to reflect the stopped state)
with a short timeout; then proceed to reuse or recreate the controller once the
stop condition is observed. Ensure the chosen approach avoids races and has a
timeout or error path to prevent deadlocks.
| if force && existing != nil { | ||
| StopBlockController(blockId) | ||
| time.Sleep(100 * time.Millisecond) | ||
| existing = getController(blockId) | ||
| } |
There was a problem hiding this comment.
Force restart: drop the sleep.
Use blocking Stop or an explicit wait instead.
- if force && existing != nil {
- StopBlockController(blockId)
- time.Sleep(100 * time.Millisecond)
- existing = getController(blockId)
- }
+ if force && existing != nil {
+ StopBlockController(blockId)
+ existing = getController(blockId)
+ }📝 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.
| if force && existing != nil { | |
| StopBlockController(blockId) | |
| time.Sleep(100 * time.Millisecond) | |
| existing = getController(blockId) | |
| } | |
| if force && existing != nil { | |
| StopBlockController(blockId) | |
| existing = getController(blockId) | |
| } |
🤖 Prompt for AI Agents
In pkg/blockcontroller/blockcontroller.go around lines 176-180, do not use
time.Sleep after calling StopBlockController; instead make StopBlockController a
blocking call (or add a StopAndWait/WaitStopped API) that returns only after the
controller is fully stopped, then call it and immediately refresh existing =
getController(blockId). Replace the sleep with the blocking Stop/Wait call (or
call StopAndWait here) so the code deterministically waits for shutdown before
proceeding.
| rlb := &ReaderLineBuffer{ | ||
| lines: make([]string, 0, maxLines), | ||
| maxLines: maxLines, | ||
| totalLineCount: 0, | ||
| reader: reader, | ||
| scanner: bufio.NewScanner(reader), | ||
| done: false, | ||
| } | ||
|
|
||
| return rlb | ||
| } |
There was a problem hiding this comment.
Protect against bufio.Scanner 64KB token limit
Long lines will trigger “token too long” and prematurely stop reads. Configure the scanner buffer to a larger max to avoid spurious EOFs/errors.
Apply this diff:
- rlb := &ReaderLineBuffer{
- lines: make([]string, 0, maxLines),
- maxLines: maxLines,
- totalLineCount: 0,
- reader: reader,
- scanner: bufio.NewScanner(reader),
- done: false,
- }
+ rlb := &ReaderLineBuffer{
+ lines: make([]string, 0, maxLines),
+ maxLines: maxLines,
+ totalLineCount: 0,
+ reader: reader,
+ scanner: func() *bufio.Scanner {
+ s := bufio.NewScanner(reader)
+ // 64KB initial, 8MB max token
+ s.Buffer(make([]byte, 64*1024), 8*1024*1024)
+ return s
+ }(),
+ done: false,
+ }📝 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.
| rlb := &ReaderLineBuffer{ | |
| lines: make([]string, 0, maxLines), | |
| maxLines: maxLines, | |
| totalLineCount: 0, | |
| reader: reader, | |
| scanner: bufio.NewScanner(reader), | |
| done: false, | |
| } | |
| return rlb | |
| } | |
| rlb := &ReaderLineBuffer{ | |
| lines: make([]string, 0, maxLines), | |
| maxLines: maxLines, | |
| totalLineCount: 0, | |
| reader: reader, | |
| scanner: func() *bufio.Scanner { | |
| s := bufio.NewScanner(reader) | |
| // 64KB initial, 8MB max token | |
| s.Buffer(make([]byte, 64*1024), 8*1024*1024) | |
| return s | |
| }(), | |
| done: false, | |
| } | |
| return rlb | |
| } |
🤖 Prompt for AI Agents
In pkg/utilds/readerlinebuffer.go around lines 25 to 35, bufio.NewScanner is
used without increasing its default 64KB token limit which causes "token too
long" errors on long lines; after creating the scanner set a larger buffer via
scanner.Buffer(make([]byte, initialSize), maxSize) (choose a sensible initial
and a larger max, e.g., initial 64KB and max several megabytes) to prevent
premature errors, and handle any error return where applicable.
| func (rlb *ReaderLineBuffer) addLine(line string) { | ||
| rlb.lock.Lock() | ||
| defer rlb.lock.Unlock() | ||
|
|
||
| rlb.totalLineCount++ | ||
|
|
||
| if len(rlb.lines) >= rlb.maxLines { | ||
| // Remove oldest line to make room | ||
| copy(rlb.lines, rlb.lines[1:]) | ||
| rlb.lines[len(rlb.lines)-1] = line | ||
| } else { | ||
| rlb.lines = append(rlb.lines, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid O(n) copy on every append at capacity
The shift-left on each add is O(n). Consider a ring buffer (head index + size) to make appends O(1).
If wanted, I can provide a minimal ring-buffer refactor with the same API.
| SSEChannels map[string]chan ssEvent // map of connectionId to SSE channel | ||
| SSEChannelsLock *sync.Mutex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer value RWMutex over pointer Mutex; use read lock for broadcasts.
Use a value sync.RWMutex (idiomatic, non-copyable) and take RLock in broadcast paths to reduce contention with register/unregister.
Apply:
- SSEChannels map[string]chan ssEvent // map of connectionId to SSE channel
- SSEChannelsLock *sync.Mutex
+ SSEChannels map[string]chan ssEvent // connectionId -> SSE channel
+ SSEChannelsMu sync.RWMutexThen update initializers and callers:
- SSEChannels: make(map[string]chan ssEvent),
- SSEChannelsLock: &sync.Mutex{},
+ SSEChannels: make(map[string]chan ssEvent),And in methods:
- c.SSEChannelsLock.Lock()
- defer c.SSEChannelsLock.Unlock()
+ c.SSEChannelsMu.Lock()
+ defer c.SSEChannelsMu.Unlock()- c.SSEChannelsLock.Lock()
- defer c.SSEChannelsLock.Unlock()
+ c.SSEChannelsMu.Lock()
+ defer c.SSEChannelsMu.Unlock()- c.SSEChannelsLock.Lock()
- defer c.SSEChannelsLock.Unlock()
+ c.SSEChannelsMu.RLock()
+ defer c.SSEChannelsMu.RUnlock()📝 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.
| SSEChannels map[string]chan ssEvent // map of connectionId to SSE channel | |
| SSEChannelsLock *sync.Mutex | |
| SSEChannels map[string]chan ssEvent // connectionId -> SSE channel | |
| SSEChannelsMu sync.RWMutex |
🤖 Prompt for AI Agents
In tsunami/engine/clientimpl.go around lines 46-47, replace the pointer
*sync.Mutex field with a value sync.RWMutex and update all call sites:
initialize nothing (use zero-value), use Lock/Unlock for register/unregister
mutations, and use RLock/RUnlock in broadcast/read paths to reduce contention;
ensure the struct is not copied (pass by pointer where needed) and update any
initialization that allocated the old mutex pointer to remove that allocation.
| c.SSEChannelsLock.Lock() | ||
| defer c.SSEChannelsLock.Unlock() | ||
|
|
||
| // Send to all registered SSE channels | ||
| for connectionId, ch := range c.SSEChannels { | ||
| select { | ||
| case ch <- event: | ||
| // Successfully sent | ||
| default: | ||
| log.Printf("SSEvent channel is full for connection %s, skipping event", connectionId) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broadcast while holding write lock; consider read lock + drop counter.
Non‑blocking send avoids stalls, but holding an exclusive lock serializes with register/unregister unnecessarily. With RWMutex, use RLock here and add a dropped‑event metric to aid troubleshooting.
Apply:
- c.SSEChannelsLock.Lock()
- defer c.SSEChannelsLock.Unlock()
+ c.SSEChannelsMu.RLock()
+ defer c.SSEChannelsMu.RUnlock()Optionally:
// increment atomic counter when skipping due to full channel🤖 Prompt for AI Agents
In tsunami/engine/clientimpl.go around lines 243 to 257 the broadcast loop
currently holds an exclusive lock (SSEChannelsLock.Lock) while doing
non‑blocking sends, which needlessly blocks register/unregister; change to use a
read lock (RLock()/RUnlock()) for the iteration so register/unregister can
proceed concurrently, and when the default case is hit (channel full) increment
an atomic dropped‑event counter (or other metrics function) to record skipped
sends; ensure the defer unlock matches the RUnlock call and the atomic counter
is safe (use sync/atomic) so no additional locking is required.
… createGoMod func as well
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tsunami/build/build.go (2)
254-265: Use the resolved Go executable; “go” might not be on PATH.verifyEnvironment finds Go outside PATH, but createGoMod and runGoBuild shell out to plain "go". This will fail when Go isn’t on PATH.
Apply these diffs:
@@ - // Run go mod tidy to clean up dependencies - tidyCmd := exec.Command("go", "mod", "tidy") + // Run go mod tidy to clean up dependencies + goPath, err := findGoExecutable() + if err != nil { + return fmt.Errorf("go command not found: %w", err) + } + tidyCmd := exec.Command(goPath, "mod", "tidy") @@ - buildCmd := exec.Command("go", args...) + goPath, err := findGoExecutable() + if err != nil { + return fmt.Errorf("go command not found: %w", err) + } + buildCmd := exec.Command(goPath, args...)Also applies to: 621-623
230-233: Validate SdkReplacePath before using it.createGoMod calls modFile.AddReplace(..., opts.SdkReplacePath, ...) and later uses opts.SdkReplacePath for a ui symlink; an empty value produces an invalid replace and a broken symlink. Add an explicit check that returns an error when opts.SdkReplacePath == "" — either in TsunamiBuildInternal before calling createGoMod or at the start of createGoMod (fail fast with a clear message).
Relevant locations: tsunami/build/build.go (AddReplace at ~line 231; ui symlink at ~line 518). Callers that already validate: tsunami/cmd/main-tsunami.go:43–49, pkg/blockcontroller/tsunamicontroller.go:147–150.
🧹 Nitpick comments (5)
tsunami/build/buildutil.go (1)
230-240: Guard against directory inputs in CopyFileIfExists.If srcPath exists but is a directory, copyFile will fail with a generic EISDIR. Make the error explicit up front.
Apply this diff:
-func CopyFileIfExists(srcPath, destPath string) (bool, error) { - if _, err := os.Stat(srcPath); err == nil { +func CopyFileIfExists(srcPath, destPath string) (bool, error) { + if info, err := os.Stat(srcPath); err == nil { + if info.IsDir() { + return false, fmt.Errorf("%s exists but is a directory", srcPath) + } if err := copyFile(srcPath, destPath); err != nil { return false, fmt.Errorf("failed to copy %s: %w", srcPath, err) } return true, nil } else if os.IsNotExist(err) { return false, nil } else { return false, fmt.Errorf("error checking %s: %w", srcPath, err) } }tsunami/build/build.go (4)
604-609: Windows: emit .exe and run the correct filename.Without .exe, CreateProcess may fail or be inconvenient on Windows.
Apply these diffs:
@@ - outputPath = "bin/app" + appName := "app" + if runtime.GOOS == "windows" { + appName += ".exe" + } + outputPath = filepath.Join("bin", appName)@@ - appPath := filepath.Join(buildEnv.TempDir, "bin", "app") + appName := "app" + if runtime.GOOS == "windows" { + appName += ".exe" + } + appPath := filepath.Join(buildEnv.TempDir, "bin", appName)Also applies to: 759-761
141-165: Don’t require NodePath to be absolute; resolve via LookPath if relative.This is unnecessarily strict and blocks “node” or “~/.nvm/.../node” style inputs. Resolve relative/custom names with exec.LookPath and validate the resolved path.
Apply this diff:
- if opts.NodePath != "" { - // Custom node path specified - verify it's absolute and executable - if !filepath.IsAbs(opts.NodePath) { - return nil, fmt.Errorf("NodePath must be an absolute path, got: %s", opts.NodePath) - } - info, err := os.Stat(opts.NodePath) + if opts.NodePath != "" { + nodePath := opts.NodePath + if !filepath.IsAbs(nodePath) { + resolved, err := exec.LookPath(nodePath) + if err != nil { + return nil, fmt.Errorf("node command not found for NodePath %q: %w", nodePath, err) + } + nodePath = resolved + } + info, err := os.Stat(nodePath) if err != nil { - return nil, fmt.Errorf("NodePath does not exist: %s: %w", opts.NodePath, err) + return nil, fmt.Errorf("NodePath does not exist: %s: %w", nodePath, err) } - if info.IsDir() { - return nil, fmt.Errorf("NodePath is a directory, not an executable: %s", opts.NodePath) + if info.IsDir() { + return nil, fmt.Errorf("NodePath is a directory, not an executable: %s", nodePath) } - // Check if file is executable (Unix-like systems) - if runtime.GOOS != "windows" && info.Mode()&0111 == 0 { - return nil, fmt.Errorf("NodePath is not executable: %s", opts.NodePath) + // Check if file is executable (Unix-like systems) + if runtime.GOOS != "windows" && info.Mode()&0111 == 0 { + return nil, fmt.Errorf("NodePath is not executable: %s", nodePath) } if verbose { - log.Printf("Using custom node path: %s", opts.NodePath) + log.Printf("Using custom node path: %s", nodePath) }
10-10: Avoid import shadowing and accept https URLs; minor cleanups.
- Alias net/url to avoid being shadowed by local vars.
- Rename local url variable.
- Accept both http and https in the listen line.
Apply these diffs:
@@ - "net/url" + neturl "net/url" @@ - port := ParseTsunamiPort(line) + port := ParseTsunamiPort(line) if port > 0 { - url := fmt.Sprintf("http://localhost:%d", port) + openURL := fmt.Sprintf("http://localhost:%d", port) if verbose { - log.Printf("Opening browser to %s", url) + log.Printf("Opening browser to %s", openURL) } - go util.OpenBrowser(url, 100*time.Millisecond) + go util.OpenBrowser(openURL, 100*time.Millisecond) browserOpened = true } @@ - urlRegex := regexp.MustCompile(`\[tsunami\] listening at (http://[^\s]+)`) + urlRegex := regexp.MustCompile(`\[tsunami\] listening at (https?://[^\s]+)`) @@ - u, err := url.Parse(matches[1]) + u, err := neturl.Parse(matches[1])Also applies to: 821-826, 839-845, 833-835
42-48: Avoid duplicating AppName() and GetAppName().One of these is sufficient; prefer the method or the function, not both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tsunami/build/build.go(22 hunks)tsunami/build/buildutil.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tsunami/build/build.go (2)
tsunami/build/buildutil.go (1)
CopyFileIfExists(230-241)tsunami/util/util.go (1)
OpenBrowser(81-102)
⏰ 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 for TestDriver.ai
- GitHub Check: Build Docsite
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (1)
tsunami/build/build.go (1)
31-40: Breaking API: BuildOpts fields changed — verify call sites & CLI wiringBuildOpts in tsunami/build/build.go now uses AppPath (Dir removed). Verify all BuildOpts literal initializers, update callers using .Dir -> .AppPath, and ensure CLI flag parsing/flags (cmd/*) that previously mapped to a "dir" value are updated.
tsunami/build/build.go (type BuildOpts):
AppPath string Verbose bool Open bool KeepTemp bool OutputFile string ScaffoldPath string SdkReplacePath string NodePath string MoveFileBack bool }
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tsunami/build/build.go (6)
49-54: Track the resolved Go binary path in BuildEnv.You resolve Go via
findGoExecutable()but later callexec.Command("go", ...). If Go isn’t on PATH (common on macOS/Homebrew), builds will fail. StoregoPathinBuildEnvand use it everywhere.type BuildEnv struct { - GoVersion string + GoVersion string + GoBin string TempDir string cleanupOnce *sync.Once }
99-180: Use the resolved Go path and return it via BuildEnv.func verifyEnvironment(verbose bool, opts BuildOpts) (*BuildEnv, error) { goPath, err := findGoExecutable() // ... - return &BuildEnv{ - GoVersion: goVersion, - cleanupOnce: &sync.Once{}, - }, nil + return &BuildEnv{ + GoVersion: goVersion, + GoBin: goPath, + cleanupOnce: &sync.Once{}, + }, nil }
182-271: Run “go mod tidy” with the resolved Go binary.-func createGoMod(tempDir, appName, goVersion string, opts BuildOpts, verbose bool) error { +func createGoMod(tempDir, appName, goVersion string, goBin string, opts BuildOpts, verbose bool) error { // ... - tidyCmd := exec.Command("go", "mod", "tidy") + tidyCmd := exec.Command(goBin, "mod", "tidy")And update the call site (see below).
403-456: Plumb GoBin to createGoMod; log improvements.- appName := GetAppName(opts.AppPath) - if err := createGoMod(tempDir, appName, buildEnv.GoVersion, opts, opts.Verbose); err != nil { + appName := GetAppName(opts.AppPath) + if err := createGoMod(tempDir, appName, buildEnv.GoVersion, buildEnv.GoBin, opts, opts.Verbose); err != nil { return buildEnv, fmt.Errorf("failed to create go.mod: %w", err) }
496-500: Build with the resolved Go binary.- if err := runGoBuild(tempDir, opts); err != nil { + if err := runGoBuild(tempDir, buildEnv.GoBin, opts); err != nil { return buildEnv, fmt.Errorf("failed to build application: %w", err) }
560-609: Windows build changes not applied — update runGoBuild/createGoMod signatures and .exe handling.
- runGoBuild still uses the old signature: func runGoBuild(tempDir string, opts BuildOpts) error (tsunami/build/build.go:560); call site remains runGoBuild(tempDir, opts) (line 497) — expected new signature (e.g., to accept goBin) but not present.
- createGoMod still uses the old signature (tsunami/build/build.go:182); call at line 467 — update if it was meant to accept goBin.
- Direct invocations of the 'go' binary remain: exec.Command("go", "mod", "tidy") (line 253) and exec.Command("go", args...) (line 588) — replace with exec.Command(goBin, ...) or otherwise ensure the configured go binary is used.
- No GOOS == "windows" /.exe suffix handling found — add explicit Windows executable-suffix logic and fix success/log paths (runGoBuild 560–609 and other affected spots, e.g., 731–741).
🧹 Nitpick comments (11)
tsunami/build/buildutil.go (3)
47-60: Case-insensitive .tsapp detection.Use
filepath.Ext+strings.EqualFoldfor portability (Windows/FS may surface upper-case file extensions).- if strings.HasSuffix(path, ".tsapp") { + if strings.EqualFold(filepath.Ext(path), ".tsapp") {
246-299: Root relpath is "."; avoid creating destDir/.When
path == srcDir,relPathbecomes"."and weMkdirAll(destDir + "/."). Harmless but untidy and can trip tools. Normalize todestDir.- relPath, err := filepath.Rel(srcDir, path) + relPath, err := filepath.Rel(srcDir, path) if err != nil { return err } - destPath := filepath.Join(destDir, relPath) + destPath := filepath.Join(destDir, relPath) + if relPath == "." { + destPath = destDir + }
380-410: Explicitly set zip compression method.Setting
header.Method = zip.Deflateavoids storing files uncompressed if an FS reports unexpected defaults.header, err := zip.FileInfoHeader(info) if err != nil { /* ... */ } header.Name = destPath +header.Method = zip.Deflatetsunami/cmd/main-tsunami.go (1)
116-142: Default package output can break when apppath is “.” or a .tsapp file.
output = filepath.Join(appPath, appName+".tsapp")fails ifapppathis.(produces./.tsapp) or ifapppathis a file. Use directory ofapppathwhen not a directory.- if output == "" { - appName := build.GetAppName(appPath) - output = filepath.Join(appPath, appName+".tsapp") - } + if output == "" { + appName := build.GetAppName(appPath) + outDir := appPath + if info, err := os.Stat(appPath); err == nil && !info.IsDir() { + outDir = filepath.Dir(appPath) + } + if base := filepath.Base(outDir); base == "." || base == string(filepath.Separator) { + // fallback to CWD if ambiguous + if cwd, err := os.Getwd(); err == nil { + outDir = cwd + } + } + output = filepath.Join(outDir, appName+".tsapp") + }tsunami/build/build.go (7)
560-609: Make runGoBuild use GoBin and produce a Windows-friendly binary name.-func runGoBuild(tempDir string, opts BuildOpts) error { +func runGoBuild(tempDir string, goBin string, opts BuildOpts) error { var outputPath string if opts.OutputFile != "" { // ... } else { binDir := filepath.Join(tempDir, "bin") if err := os.MkdirAll(binDir, 0755); err != nil { /* ... */ } - outputPath = "bin/app" + exe := "app" + if runtime.GOOS == "windows" { + exe += ".exe" + } + outputPath = filepath.Join("bin", exe) } // ... - buildCmd := exec.Command("go", args...) + buildCmd := exec.Command(goBin, args...)
731-741: Use the correct binary name on Windows when running the app.- appBinPath := filepath.Join(buildEnv.TempDir, "bin", "app") + exe := "app" + if runtime.GOOS == "windows" { + exe += ".exe" + } + appBinPath := filepath.Join(buildEnv.TempDir, "bin", exe)
787-803: Avoid shadowing the imported “url” package.Rename local variable to improve clarity and prevent shadowing warnings.
- url := fmt.Sprintf("http://localhost:%d", port) - if verbose { log.Printf("Opening browser to %s", url) } - go util.OpenBrowser(url, 100*time.Millisecond) + openURL := fmt.Sprintf("http://localhost:%d", port) + if verbose { log.Printf("Opening browser to %s", openURL) } + go util.OpenBrowser(openURL, 100*time.Millisecond)
140-174: NodePath UX: allow non-absolute values.Requiring absolute
NodePathis strict; if a user supplies “node” or a tool-managed shim, resolve it withexec.LookPathbefore erroring. Keeps security checks but improves ergonomics.- if !filepath.IsAbs(opts.NodePath) { - return nil, fmt.Errorf("NodePath must be an absolute path, got: %s", opts.NodePath) - } + if !filepath.IsAbs(opts.NodePath) { + if lp, err := exec.LookPath(opts.NodePath); err == nil { + opts.NodePath = lp + } else { + return nil, fmt.Errorf("NodePath not absolute and not found in PATH: %s", opts.NodePath) + } + }
612-621: Consider timeouts for external commands.Long-running
tailwindinvocations can hang; considerexec.CommandContextwith a sensible timeout.
252-269: Log-only “NOT cleaning tempdir”.Printing “NOT cleaning tempdir” even when not verbose can surprise users. Gate by
verbose.- if keepTemp || be.TempDir == "" { - log.Printf("NOT cleaning tempdir\n") + if keepTemp || be.TempDir == "" { + if verbose { + log.Printf("NOT cleaning tempdir") + } return }
393-401: Nil-safe defer is fine; optional guard.Calling
defer buildEnv.cleanupTempDir(...)before checkingerris nil-safe due to the method’s nil guard. Optional: wrap inif buildEnv != nilto appease linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tsunami/.gitignore(1 hunks)tsunami/build/build.go(22 hunks)tsunami/build/buildutil.go(5 hunks)tsunami/cmd/main-tsunami.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tsunami/cmd/main-tsunami.go (2)
tsunami/build/build.go (3)
BuildOpts(32-42)GetAppName(44-47)MakeAppPackage(911-964)tsunami/build/buildutil.go (1)
NewDirFS(19-21)
tsunami/build/build.go (2)
tsunami/build/buildutil.go (3)
CopyFileFromFS(177-212)CopyFileIfExists(164-175)DirFS(14-17)tsunami/util/util.go (1)
OpenBrowser(81-102)
⏰ 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 (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (1)
tsunami/.gitignore (1)
1-2: LGTM: ignore build artifacts.Ignoring
bin/and*.tsappis appropriate for generated outputs.
tsunami/build/build.go
Outdated
| appFS, canWrite, err := pathToFS(opts.AppPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("bad app path: %w", err) | ||
| } | ||
|
|
||
| if err := verifyAppPathFs(appFS); err != nil { | ||
| return nil, fmt.Errorf("bad app path: %w", err) | ||
| } | ||
|
|
||
| scaffoldFS, _, err := pathToFS(opts.ScaffoldPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("bad scaffold path: %w", err) | ||
| } | ||
|
|
||
| if err := verifyScaffoldPath(opts.ScaffoldPath); err != nil { | ||
| if err := verifyScaffoldFs(scaffoldFS); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Defer-close FS closers returned by pathToFS.
With pathToFS returning io.Closer, ensure they’re closed to release zip handles.
- appFS, canWrite, err := pathToFS(opts.AppPath)
+ appFS, appCloser, canWrite, err := pathToFS(opts.AppPath)
if err != nil { /* ... */ }
+ if appCloser != nil { defer appCloser.Close() }
- scaffoldFS, _, err := pathToFS(opts.ScaffoldPath)
+ scaffoldFS, scaffoldCloser, _, err := pathToFS(opts.ScaffoldPath)
if err != nil { /* ... */ }
+ if scaffoldCloser != nil { defer scaffoldCloser.Close() }📝 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.
| appFS, canWrite, err := pathToFS(opts.AppPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("bad app path: %w", err) | |
| } | |
| if err := verifyAppPathFs(appFS); err != nil { | |
| return nil, fmt.Errorf("bad app path: %w", err) | |
| } | |
| scaffoldFS, _, err := pathToFS(opts.ScaffoldPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("bad scaffold path: %w", err) | |
| } | |
| if err := verifyScaffoldPath(opts.ScaffoldPath); err != nil { | |
| if err := verifyScaffoldFs(scaffoldFS); err != nil { | |
| return nil, err | |
| appFS, appCloser, canWrite, err := pathToFS(opts.AppPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("bad app path: %w", err) | |
| } | |
| if appCloser != nil { | |
| defer appCloser.Close() | |
| } | |
| if err := verifyAppPathFs(appFS); err != nil { | |
| return nil, fmt.Errorf("bad app path: %w", err) | |
| } | |
| scaffoldFS, scaffoldCloser, _, err := pathToFS(opts.ScaffoldPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("bad scaffold path: %w", err) | |
| } | |
| if scaffoldCloser != nil { | |
| defer scaffoldCloser.Close() | |
| } | |
| if err := verifyScaffoldFs(scaffoldFS); err != nil { | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In tsunami/build/build.go around lines 409-424, pathToFS returns an io.Closer
that is currently ignored/discarded; capture those closers (e.g., appCloser,
scaffoldCloser) when calling pathToFS, and after confirming err == nil
immediately defer a non-nil close (defer func(){ if appCloser != nil { _ =
appCloser.Close() } }()) for each to ensure zip handles are released; do this
for both the app and scaffold FS calls (defer after successful pathToFS return,
before returning from this function) and ignore Close errors if desired.
| func copyScaffoldFS(scaffoldFS fs.FS, destDir string, verbose bool) (int, error) { | ||
| fileCount := 0 | ||
|
|
||
| // Handle node_modules directory - prefer symlink if possible, otherwise copy | ||
| if _, err := fs.Stat(scaffoldFS, "node_modules"); err == nil { | ||
| destPath := filepath.Join(destDir, "node_modules") | ||
|
|
||
| // Try to create symlink if we have DirFS | ||
| if dirFS, ok := scaffoldFS.(DirFS); ok { | ||
| srcPath := dirFS.JoinOS("node_modules") | ||
| if err := os.Symlink(srcPath, destPath); err != nil { | ||
| return 0, fmt.Errorf("failed to create symlink for node_modules: %w", err) | ||
| } | ||
| if verbose { | ||
| log.Printf("Symlinked node_modules directory\n") | ||
| } | ||
| fileCount++ | ||
| } else { | ||
| // Fallback to recursive copy | ||
| dirCount, err := copyDirFromFS(scaffoldFS, "node_modules", destPath, false) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to copy node_modules directory: %w", err) | ||
| } | ||
| if verbose { | ||
| log.Printf("Opening browser to %s", url) | ||
| log.Printf("Copied node_modules directory (%d files)\n", dirCount) | ||
| } | ||
| go util.OpenBrowser(url, 100*time.Millisecond) | ||
| browserOpened = true | ||
| fileCount += dirCount | ||
| } | ||
| } else if !os.IsNotExist(err) { | ||
| return 0, fmt.Errorf("error checking node_modules: %w", err) | ||
| } | ||
|
|
||
| // Copy package files instead of symlinking | ||
| packageFiles := []string{"package.json", "package-lock.json"} | ||
| for _, fileName := range packageFiles { | ||
| destPath := filepath.Join(destDir, fileName) | ||
|
|
||
| // Check if source exists in FS | ||
| if _, err := fs.Stat(scaffoldFS, fileName); err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue // Skip if doesn't exist | ||
| } | ||
| return 0, fmt.Errorf("error checking %s: %w", fileName, err) | ||
| } | ||
|
|
||
| // Copy file from FS | ||
| if err := CopyFileFromFS(scaffoldFS, fileName, destPath); err != nil { | ||
| return 0, fmt.Errorf("failed to copy %s: %w", fileName, err) | ||
| } | ||
| fileCount++ | ||
| } | ||
|
|
||
| // Copy dist directory using FS | ||
| distDestPath := filepath.Join(destDir, "dist") | ||
| dirCount, err := copyDirFromFS(scaffoldFS, "dist", distDestPath, false) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to copy dist directory: %w", err) | ||
| } | ||
| fileCount += dirCount | ||
|
|
||
| // Copy files by pattern (*.go, *.md, *.json, tailwind.css) | ||
| patterns := []string{"*.go", "*.md", "*.json", "tailwind.css"} | ||
|
|
||
| for _, pattern := range patterns { | ||
| matches, err := fs.Glob(scaffoldFS, pattern) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to glob pattern %s: %w", pattern, err) | ||
| } | ||
|
|
||
| for _, match := range matches { | ||
| destPath := filepath.Join(destDir, match) | ||
| if err := CopyFileFromFS(scaffoldFS, match, destPath); err != nil { | ||
| return 0, fmt.Errorf("failed to copy %s: %w", match, err) | ||
| } | ||
| fileCount++ | ||
| } | ||
| } | ||
|
|
||
| return fileCount, nil | ||
| } |
There was a problem hiding this comment.
Symlink fallback for node_modules to improve cross-platform reliability.
Symlinks often fail on Windows or restricted filesystems. Fall back to copying when symlink creation fails.
if _, err := fs.Stat(scaffoldFS, "node_modules"); err == nil {
destPath := filepath.Join(destDir, "node_modules")
if dirFS, ok := scaffoldFS.(DirFS); ok {
srcPath := dirFS.JoinOS("node_modules")
- if err := os.Symlink(srcPath, destPath); err != nil {
- return 0, fmt.Errorf("failed to create symlink for node_modules: %w", err)
- }
- if verbose {
- log.Printf("Symlinked node_modules directory\n")
- }
- fileCount++
+ if err := os.Symlink(srcPath, destPath); err != nil {
+ if verbose {
+ log.Printf("Symlink failed (%v); falling back to copy for node_modules", err)
+ }
+ dirCount, cerr := copyDirFromFS(scaffoldFS, "node_modules", destPath, false)
+ if cerr != nil {
+ return 0, fmt.Errorf("failed to copy node_modules directory after symlink failure: %w", cerr)
+ }
+ if verbose {
+ log.Printf("Copied node_modules directory (%d files)\n", dirCount)
+ }
+ fileCount += dirCount
+ } else {
+ if verbose {
+ log.Printf("Symlinked node_modules directory\n")
+ }
+ fileCount++
+ }
} else {
// Fallback to recursive copy
dirCount, err := copyDirFromFS(scaffoldFS, "node_modules", destPath, false)
if err != nil {
return 0, fmt.Errorf("failed to copy node_modules directory: %w", err)
}
if verbose {
log.Printf("Copied node_modules directory (%d files)\n", dirCount)
}
fileCount += dirCount
}
} else if !os.IsNotExist(err) {
return 0, fmt.Errorf("error checking node_modules: %w", err)
}📝 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 copyScaffoldFS(scaffoldFS fs.FS, destDir string, verbose bool) (int, error) { | |
| fileCount := 0 | |
| // Handle node_modules directory - prefer symlink if possible, otherwise copy | |
| if _, err := fs.Stat(scaffoldFS, "node_modules"); err == nil { | |
| destPath := filepath.Join(destDir, "node_modules") | |
| // Try to create symlink if we have DirFS | |
| if dirFS, ok := scaffoldFS.(DirFS); ok { | |
| srcPath := dirFS.JoinOS("node_modules") | |
| if err := os.Symlink(srcPath, destPath); err != nil { | |
| return 0, fmt.Errorf("failed to create symlink for node_modules: %w", err) | |
| } | |
| if verbose { | |
| log.Printf("Symlinked node_modules directory\n") | |
| } | |
| fileCount++ | |
| } else { | |
| // Fallback to recursive copy | |
| dirCount, err := copyDirFromFS(scaffoldFS, "node_modules", destPath, false) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to copy node_modules directory: %w", err) | |
| } | |
| if verbose { | |
| log.Printf("Opening browser to %s", url) | |
| log.Printf("Copied node_modules directory (%d files)\n", dirCount) | |
| } | |
| go util.OpenBrowser(url, 100*time.Millisecond) | |
| browserOpened = true | |
| fileCount += dirCount | |
| } | |
| } else if !os.IsNotExist(err) { | |
| return 0, fmt.Errorf("error checking node_modules: %w", err) | |
| } | |
| // Copy package files instead of symlinking | |
| packageFiles := []string{"package.json", "package-lock.json"} | |
| for _, fileName := range packageFiles { | |
| destPath := filepath.Join(destDir, fileName) | |
| // Check if source exists in FS | |
| if _, err := fs.Stat(scaffoldFS, fileName); err != nil { | |
| if os.IsNotExist(err) { | |
| continue // Skip if doesn't exist | |
| } | |
| return 0, fmt.Errorf("error checking %s: %w", fileName, err) | |
| } | |
| // Copy file from FS | |
| if err := CopyFileFromFS(scaffoldFS, fileName, destPath); err != nil { | |
| return 0, fmt.Errorf("failed to copy %s: %w", fileName, err) | |
| } | |
| fileCount++ | |
| } | |
| // Copy dist directory using FS | |
| distDestPath := filepath.Join(destDir, "dist") | |
| dirCount, err := copyDirFromFS(scaffoldFS, "dist", distDestPath, false) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to copy dist directory: %w", err) | |
| } | |
| fileCount += dirCount | |
| // Copy files by pattern (*.go, *.md, *.json, tailwind.css) | |
| patterns := []string{"*.go", "*.md", "*.json", "tailwind.css"} | |
| for _, pattern := range patterns { | |
| matches, err := fs.Glob(scaffoldFS, pattern) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to glob pattern %s: %w", pattern, err) | |
| } | |
| for _, match := range matches { | |
| destPath := filepath.Join(destDir, match) | |
| if err := CopyFileFromFS(scaffoldFS, match, destPath); err != nil { | |
| return 0, fmt.Errorf("failed to copy %s: %w", match, err) | |
| } | |
| fileCount++ | |
| } | |
| } | |
| return fileCount, nil | |
| } | |
| func copyScaffoldFS(scaffoldFS fs.FS, destDir string, verbose bool) (int, error) { | |
| fileCount := 0 | |
| // Handle node_modules directory - prefer symlink if possible, otherwise copy | |
| if _, err := fs.Stat(scaffoldFS, "node_modules"); err == nil { | |
| destPath := filepath.Join(destDir, "node_modules") | |
| // Try to create symlink if we have DirFS | |
| if dirFS, ok := scaffoldFS.(DirFS); ok { | |
| srcPath := dirFS.JoinOS("node_modules") | |
| if err := os.Symlink(srcPath, destPath); err != nil { | |
| if verbose { | |
| log.Printf("Symlink failed (%v); falling back to copy for node_modules", err) | |
| } | |
| dirCount, cerr := copyDirFromFS(scaffoldFS, "node_modules", destPath, false) | |
| if cerr != nil { | |
| return 0, fmt.Errorf("failed to copy node_modules directory after symlink failure: %w", cerr) | |
| } | |
| if verbose { | |
| log.Printf("Copied node_modules directory (%d files)\n", dirCount) | |
| } | |
| fileCount += dirCount | |
| } else { | |
| if verbose { | |
| log.Printf("Symlinked node_modules directory\n") | |
| } | |
| fileCount++ | |
| } | |
| } else { | |
| // Fallback to recursive copy | |
| dirCount, err := copyDirFromFS(scaffoldFS, "node_modules", destPath, false) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to copy node_modules directory: %w", err) | |
| } | |
| if verbose { | |
| log.Printf("Copied node_modules directory (%d files)\n", dirCount) | |
| } | |
| fileCount += dirCount | |
| } | |
| } else if !os.IsNotExist(err) { | |
| return 0, fmt.Errorf("error checking node_modules: %w", err) | |
| } | |
| // Copy package files instead of symlinking | |
| packageFiles := []string{"package.json", "package-lock.json"} | |
| for _, fileName := range packageFiles { | |
| destPath := filepath.Join(destDir, fileName) | |
| // Check if source exists in FS | |
| if _, err := fs.Stat(scaffoldFS, fileName); err != nil { | |
| if os.IsNotExist(err) { | |
| continue // Skip if doesn't exist | |
| } | |
| return 0, fmt.Errorf("error checking %s: %w", fileName, err) | |
| } | |
| // Copy file from FS | |
| if err := CopyFileFromFS(scaffoldFS, fileName, destPath); err != nil { | |
| return 0, fmt.Errorf("failed to copy %s: %w", fileName, err) | |
| } | |
| fileCount++ | |
| } | |
| // Copy dist directory using FS | |
| distDestPath := filepath.Join(destDir, "dist") | |
| dirCount, err := copyDirFromFS(scaffoldFS, "dist", distDestPath, false) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to copy dist directory: %w", err) | |
| } | |
| fileCount += dirCount | |
| // Copy files by pattern (*.go, *.md, *.json, tailwind.css) | |
| patterns := []string{"*.go", "*.md", "*.json", "tailwind.css"} | |
| for _, pattern := range patterns { | |
| matches, err := fs.Glob(scaffoldFS, pattern) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to glob pattern %s: %w", pattern, err) | |
| } | |
| for _, match := range matches { | |
| destPath := filepath.Join(destDir, match) | |
| if err := CopyFileFromFS(scaffoldFS, match, destPath); err != nil { | |
| return 0, fmt.Errorf("failed to copy %s: %w", match, err) | |
| } | |
| fileCount++ | |
| } | |
| } | |
| return fileCount, nil | |
| } |
🤖 Prompt for AI Agents
In tsunami/build/build.go around lines 830 to 909, the code currently fails the
whole operation if os.Symlink for node_modules fails (which breaks on
Windows/restricted FS); change the logic to treat symlink failure as a non-fatal
condition and fall back to copying the node_modules directory from scaffoldFS
into destDir using copyDirFromFS, logging the symlink error when verbose and
continuing; ensure fileCount is updated with the number of files copied in the
fallback and only return an error if the copy itself fails.
tsunami/build/buildutil.go
Outdated
| func pathToFS(path string) (fs.FS, bool, error) { | ||
| if path == "" { | ||
| return nil, false, fmt.Errorf("directory path cannot be empty") | ||
| } | ||
|
|
||
| // Check if path exists | ||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, false, fmt.Errorf("path %q does not exist", path) | ||
| } | ||
| return nil, false, fmt.Errorf("error accessing path %q: %w", path, err) | ||
| } | ||
|
|
||
| // Check if it's a .tsapp file (zip archive) | ||
| if strings.HasSuffix(path, ".tsapp") { | ||
| if info.IsDir() { | ||
| return nil, false, fmt.Errorf("%q is a directory, but .tsapp files must be zip archives", path) | ||
| } | ||
|
|
||
| // Open as zip file | ||
| zipReader, err := zip.OpenReader(path) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("failed to open .tsapp file %q as zip archive: %w", path, err) | ||
| } | ||
|
|
||
| // Return zip filesystem (not writable) | ||
| return zipReader, false, nil | ||
| } | ||
|
|
||
| // Handle regular directories | ||
| if !info.IsDir() { | ||
| return nil, false, fmt.Errorf("%q is not a directory", path) | ||
| } | ||
|
|
||
| // Check if directory is writable by checking permissions | ||
| canWrite := info.Mode().Perm()&0200 != 0 // Check if owner has write permission | ||
|
|
||
| return NewDirFS(path), canWrite, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close the .tsapp zip to avoid file-descriptor leaks; extend pathToFS to return an io.Closer.
When opening a .tsapp, zip.OpenReader returns a *zip.ReadCloser that must be closed. Returning only fs.FS drops the closer, leaking descriptors and locking files (notably on Windows).
Apply this change and adapt call sites:
- func pathToFS(path string) (fs.FS, bool, error) {
+ func pathToFS(path string) (fs.FS, io.Closer, bool, error) {
if path == "" {
- return nil, false, fmt.Errorf("directory path cannot be empty")
+ return nil, nil, false, fmt.Errorf("directory path cannot be empty")
}
// ...
- if strings.HasSuffix(path, ".tsapp") {
+ if strings.EqualFold(filepath.Ext(path), ".tsapp") {
if info.IsDir() {
- return nil, false, fmt.Errorf("%q is a directory, but .tsapp files must be zip archives", path)
+ return nil, nil, false, fmt.Errorf("%q is a directory, but .tsapp files must be zip archives", path)
}
- zipReader, err := zip.OpenReader(path)
+ zipReader, err := zip.OpenReader(path)
if err != nil {
- return nil, false, fmt.Errorf("failed to open .tsapp file %q as zip archive: %w", path, err)
+ return nil, nil, false, fmt.Errorf("failed to open .tsapp file %q as zip archive: %w", path, err)
}
- return zipReader, false, nil
+ return zipReader, zipReader, false, nil
}
// regular dir
if !info.IsDir() {
- return nil, false, fmt.Errorf("%q is not a directory", path)
+ return nil, nil, false, fmt.Errorf("%q is not a directory", path)
}
canWrite := info.Mode().Perm()&0200 != 0
- return NewDirFS(path), canWrite, nil
+ return NewDirFS(path), nil, canWrite, nil
}And in tsunami/build/build.go (see comments there) defer-close the returned closers.
📝 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 pathToFS(path string) (fs.FS, bool, error) { | |
| if path == "" { | |
| return nil, false, fmt.Errorf("directory path cannot be empty") | |
| } | |
| // Check if path exists | |
| info, err := os.Stat(path) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return nil, false, fmt.Errorf("path %q does not exist", path) | |
| } | |
| return nil, false, fmt.Errorf("error accessing path %q: %w", path, err) | |
| } | |
| // Check if it's a .tsapp file (zip archive) | |
| if strings.HasSuffix(path, ".tsapp") { | |
| if info.IsDir() { | |
| return nil, false, fmt.Errorf("%q is a directory, but .tsapp files must be zip archives", path) | |
| } | |
| // Open as zip file | |
| zipReader, err := zip.OpenReader(path) | |
| if err != nil { | |
| return nil, false, fmt.Errorf("failed to open .tsapp file %q as zip archive: %w", path, err) | |
| } | |
| // Return zip filesystem (not writable) | |
| return zipReader, false, nil | |
| } | |
| // Handle regular directories | |
| if !info.IsDir() { | |
| return nil, false, fmt.Errorf("%q is not a directory", path) | |
| } | |
| // Check if directory is writable by checking permissions | |
| canWrite := info.Mode().Perm()&0200 != 0 // Check if owner has write permission | |
| return NewDirFS(path), canWrite, nil | |
| } | |
| func pathToFS(path string) (fs.FS, io.Closer, bool, error) { | |
| if path == "" { | |
| return nil, nil, false, fmt.Errorf("directory path cannot be empty") | |
| } | |
| // Check if path exists | |
| info, err := os.Stat(path) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return nil, nil, false, fmt.Errorf("path %q does not exist", path) | |
| } | |
| return nil, nil, false, fmt.Errorf("error accessing path %q: %w", path, err) | |
| } | |
| // Check if it's a .tsapp file (zip archive) | |
| if strings.EqualFold(filepath.Ext(path), ".tsapp") { | |
| if info.IsDir() { | |
| return nil, nil, false, fmt.Errorf("%q is a directory, but .tsapp files must be zip archives", path) | |
| } | |
| // Open as zip file | |
| zipReader, err := zip.OpenReader(path) | |
| if err != nil { | |
| return nil, nil, false, fmt.Errorf("failed to open .tsapp file %q as zip archive: %w", path, err) | |
| } | |
| // Return zip filesystem (not writable) | |
| return zipReader, zipReader, false, nil | |
| } | |
| // Handle regular directories | |
| if !info.IsDir() { | |
| return nil, nil, false, fmt.Errorf("%q is not a directory", path) | |
| } | |
| // Check if directory is writable by checking permissions | |
| canWrite := info.Mode().Perm()&0200 != 0 // Check if owner has write permission | |
| return NewDirFS(path), nil, canWrite, nil | |
| } |
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 (2)
tsunami/build/build.go (2)
401-409: Nil deref: defer on possibly nil buildEnv.If TsunamiBuildInternal fails early, deferring buildEnv.cleanupTempDir panics.
-func TsunamiBuild(opts BuildOpts) error { - buildEnv, err := TsunamiBuildInternal(opts) - defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) +func TsunamiBuild(opts BuildOpts) error { + buildEnv, err := TsunamiBuildInternal(opts) + if buildEnv != nil { + defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + }Apply the same guard in TsunamiRun.
- 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) + }
99-180: Use discovered Go executable; current code falls back to bare “go”.verifyEnvironment finds goPath, but createGoMod/runGoBuild still execute "go", failing when Go isn’t on PATH.
@@ -type BuildEnv struct { - GoVersion string +type BuildEnv struct { + GoVersion string + GoExe string @@ -func verifyEnvironment(verbose bool, opts BuildOpts) (*BuildEnv, error) { +func verifyEnvironment(verbose bool, opts BuildOpts) (*BuildEnv, error) { @@ - return &BuildEnv{ - GoVersion: goVersion, + return &BuildEnv{ + GoVersion: goVersion, + GoExe: goPath, cleanupOnce: &sync.Once{}, }, nil } @@ -func createGoMod(tempDir, appName, goVersion string, opts BuildOpts, verbose bool) error { +func createGoMod(tempDir, appName string, be *BuildEnv, opts BuildOpts, verbose bool) error { @@ - if err := modFile.AddGoStmt(goVersion); err != nil { + if err := modFile.AddGoStmt(be.GoVersion); err != nil { return fmt.Errorf("failed to add go version: %w", err) } @@ - tidyCmd := exec.Command("go", "mod", "tidy") + tidyCmd := exec.Command(be.GoExe, "mod", "tidy") @@ - if err := createGoMod(tempDir, appName, buildEnv.GoVersion, opts, opts.Verbose); err != nil { + if err := createGoMod(tempDir, appName, buildEnv, opts, opts.Verbose); err != nil { return buildEnv, fmt.Errorf("failed to create go.mod: %w", err) } @@ -func runGoBuild(tempDir string, opts BuildOpts) error { +func runGoBuild(tempDir string, be *BuildEnv, opts BuildOpts) error { @@ - args := append([]string{"build", "-o", outputPath}, goFiles...) - buildCmd := exec.Command("go", args...) + args := append([]string{"build", "-o", outputPath}, goFiles...) + buildCmd := exec.Command(be.GoExe, args...) @@ - if err := runGoBuild(tempDir, opts); err != nil { + if err := runGoBuild(tempDir, buildEnv, opts); err != nil { return buildEnv, fmt.Errorf("failed to build application: %w", err) }Also applies to: 182-271, 473-477, 568-618
♻️ Duplicate comments (5)
emain/emain-util.ts (1)
110-133: Don’t open non‑http(s) URLs from tsunami frames; also allow 127.0.0.1/::1 and optionally https.Current branch will
openExternaleven forfile:,data:,javascript:etc. iframes namedtsunami:*, which is unsafe. Also onlylocalhostandhttp:are allowed; include127.0.0.1,::1, andhttps:.Apply:
- if (event.frame.name != null && event.frame.name.startsWith("tsunami:")) { - // Parse port from frame name: tsunami:[port]:[blockid] - const nameParts = event.frame.name.split(":"); - const expectedPort = nameParts.length >= 2 ? nameParts[1] : null; - - try { - const tsunamiUrl = new URL(url); - if ( - tsunamiUrl.protocol === "http:" && - tsunamiUrl.hostname === "localhost" && - expectedPort && - tsunamiUrl.port === expectedPort - ) { - // allowed - return; - } - // If navigation is not to expected port, open externally - event.preventDefault(); - electron.shell.openExternal(url); - return; - } catch (e) { - // Invalid URL, fall through to prevent navigation - } - } + if (event.frame.name != null && event.frame.name.startsWith("tsunami:")) { + // Parse port from frame name: tsunami:[port]:[blockid] + const nameParts = event.frame.name.split(":"); + const expectedPort = nameParts.length >= 2 ? nameParts[1] : null; + + try { + const u = new URL(url); + const allowedProtocol = u.protocol === "http:" || u.protocol === "https:"; + const isLocalHost = u.hostname === "localhost" || u.hostname === "127.0.0.1" || u.hostname === "::1"; + const portMatches = !!expectedPort && u.port === expectedPort; + if (allowedProtocol && isLocalHost && portMatches) { + return; // allowed + } + event.preventDefault(); + if (allowedProtocol) { + // Remote or wrong-port http(s): open externally + electron.shell.openExternal(url); + } else { + // Block non-http(s) protocols + console.log("frame navigation blocked (tsunami, non-http(s) protocol):", url); + } + return; + } catch { + // Invalid URL: block below + } + }tsunami/build/build.go (2)
838-867: Symlink-only node_modules breaks on Windows/restricted FS; fall back to copy.On symlink error we should copy instead of failing.
- if err := os.Symlink(srcPath, destPath); err != nil { - return 0, fmt.Errorf("failed to create symlink for node_modules: %w", err) - } - if verbose { - log.Printf("Symlinked node_modules directory\n") - } - fileCount++ + if err := os.Symlink(srcPath, destPath); err != nil { + if verbose { + log.Printf("Symlink for node_modules failed (%v); falling back to copy", err) + } + dirCount, cerr := copyDirFromFS(scaffoldFS, "node_modules", destPath, false) + if cerr != nil { + return 0, fmt.Errorf("failed to copy node_modules after symlink failure: %w", cerr) + } + if verbose { + log.Printf("Copied node_modules directory (%d files)", dirCount) + } + fileCount += dirCount + } else { + if verbose { + log.Printf("Symlinked node_modules directory") + } + fileCount++ + }
417-425: Potential FS handle leak: closers from pathToFS likely ignored.If pathToFS returns an io.Closer, capture and defer Close() to release zip handles.
Run this to confirm the current signature and call sites:
#!/bin/bash # Inspect pathToFS signature and usages rg -nP 'func\s+pathToFS\s*\(' tsunami/build -A2 -B2 rg -n 'pathToFS\(' tsunami/build | sed -n '1,200p'Also applies to: 426-434
pkg/blockcontroller/tsunamicontroller.go (2)
103-128: Build cache freshness check is too narrow (only app.go).Changes to other .go files or static assets won’t invalidate cache; compute latest mtime across app dir and static/ (or a content hash) and compare.
372-401: Possible goroutine leak and unclear EOF handling when scanning stderr.errChan and portChan are size 1; send can block and leak goroutine; EOF should map to a clearer error; also log early stderr lines.
- go func() { - for { - line, err := stderrBuffer.ReadLine() - if err != nil { - errChan <- fmt.Errorf("stderr buffer error: %w", err) - return - } - port := build.ParseTsunamiPort(line) - if port > 0 { - portChan <- port - return - } - } - }() + go func() { + for { + line, err := stderrBuffer.ReadLine() + if err != nil { + select { + case errChan <- fmt.Errorf("tsunami stderr closed before listening message (EOF)"): + default: + } + return + } + log.Printf("[tsunami:%s] %s\n", appName, line) + if port := build.ParseTsunamiPort(line); port > 0 { + select { + case portChan <- port: + default: + } + return + } + } + }() @@ - errChan <- fmt.Errorf("tsunami process died before emitting listening message") + select { + case errChan <- fmt.Errorf("tsunami process died before emitting listening message"): + default: + }
🧹 Nitpick comments (7)
tsunami/frontend/src/vdom.tsx (1)
145-169: Dead constants (IdAttributes, SvgUrlIdAttributes) are unused.Either wire them into prop conversion (e.g., to rewrite url(#id) safely) or remove to avoid drift.
frontend/app/view/tsunami/tsunami.tsx (3)
31-44: Console noise in production.Remove or guard the “tsunami-status” console.log behind a verbose flag to keep devtools clean.
- updateShellProcStatus(fullStatus: BlockControllerRuntimeStatus) { - console.log("tsunami-status", fullStatus); + updateShellProcStatus(fullStatus: BlockControllerRuntimeStatus) { + // if (process.env.NODE_ENV !== "production") console.debug("tsunami-status", fullStatus);
31-33: Atom type should reflect nullable initial state.Initialize with a nullable type to avoid accidental misuse.
- this.shellProcFullStatus = jotai.atom(null) as jotai.PrimitiveAtom<BlockControllerRuntimeStatus>; + this.shellProcFullStatus = jotai.atom<BlockControllerRuntimeStatus | null>(null);
148-151: Harden iframe: sandbox/allow + lazy loading.Limit capabilities while keeping same-origin for app messaging.
- return <iframe src={iframeUrl} className="w-full h-full border-0" title="Tsunami Application" name={`tsunami:${shellProcFullStatus.tsunamiport}:${model.blockId}`} />; + return ( + <iframe + src={iframeUrl} + className="w-full h-full border-0" + title="Tsunami Application" + name={`tsunami:${shellProcFullStatus.tsunamiport}:${model.blockId}`} + sandbox="allow-scripts allow-forms allow-modals allow-same-origin" + allow="clipboard-read; clipboard-write; fullscreen" + loading="lazy" + referrerPolicy="no-referrer" + /> + );tsunami/build/build.go (1)
485-495: UI symlink creation may fail; add copy fallback.Symlink to SDK ui will fail on Windows or without privileges; copy directory as fallback.
- if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { - return buildEnv, fmt.Errorf("failed to create ui symlink: %w", err) - } + if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { + if opts.Verbose { + log.Printf("UI symlink failed (%v); copying ui directory", err) + } + if err := copyDir(uiTargetPath, uiLinkPath); err != nil { + return buildEnv, fmt.Errorf("failed to copy ui directory: %w", err) + } + }Note: use an existing copyDir helper if available; otherwise implement alongside other FS utils.
pkg/blockcontroller/tsunamicontroller.go (2)
216-219: Executable bit check may be brittle on macOS Gatekeeper.Consider attempting exec and reporting the actual error instead of preemptively failing on mode bits.
251-277: Stop() should best-effort wait for process reaping before returning.Optional: wait on WaitCh with short timeout to avoid zombie windows and racey status.
if c.tsunamiProc.Cmd.Process != nil { c.tsunamiProc.Cmd.Process.Kill() } @@ - c.tsunamiProc = nil + done := c.tsunamiProc.WaitCh + c.tsunamiProc = nil + select { + case <-done: + case <-time.After(2 * time.Second): + }(Add time import.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emain/emain-util.ts(3 hunks)frontend/app/view/tsunami/tsunami.tsx(1 hunks)pkg/blockcontroller/tsunamicontroller.go(1 hunks)tsunami/build/build.go(22 hunks)tsunami/frontend/src/vdom.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tsunami/build/build.go (2)
tsunami/build/buildutil.go (3)
CopyFileFromFS(177-212)CopyFileIfExists(164-175)DirFS(14-17)tsunami/util/util.go (1)
OpenBrowser(81-102)
pkg/blockcontroller/tsunamicontroller.go (8)
pkg/utilds/readerlinebuffer.go (2)
ReaderLineBuffer(9-18)MakeReaderLineBuffer(20-35)pkg/wavebase/wavebase.go (5)
IsDevMode(106-108)GetHomeDir(130-136)TryMkdirs(207-223)ExpandHomeDir(138-152)GetWaveAppElectronExecPath(126-128)tsunami/build/build.go (5)
GetAppName(44-47)GetAppModTime(287-302)BuildOpts(32-42)TsunamiBuild(401-409)ParseTsunamiPort(813-836)pkg/waveobj/metaconsts.go (3)
MetaKey_TsunamiScaffoldPath(120-120)MetaKey_TsunamiSdkReplacePath(118-118)MetaKey_TsunamiAppPath(119-119)pkg/blockcontroller/blockcontroller.go (7)
Status_Running(34-34)Status_Done(35-35)BlockControllerRuntimeStatus(54-61)SendInput(250-256)BlockInputUnion(48-52)Controller(64-69)Status_Init(36-36)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (2)
WaveEvent(21-27)Event_ControllerStatus(12-12)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)
frontend/app/view/tsunami/tsunami.tsx (2)
pkg/blockcontroller/blockcontroller.go (1)
BlockControllerRuntimeStatus(54-61)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)
⏰ 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: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (2)
emain/emain-util.ts (2)
65-71: Dev hot‑reload allowlist: LGTM.Readable and scoped to known Vite ports.
8-12: Approve — exported Electron exec path is propagated and consumed.
emain/emain-wavesrv.ts sets envCopy[WaveAppElectronExecPath] = getElectronExecPath(); pkg/wavebase/wavebase.go reads WAVETERM_ELECTRONEXECPATH into AppElectronExecPath_VarCache and exposes GetWaveAppElectronExecPath(); pkg/blockcontroller/tsunamicontroller.go checks for non-empty and errors if unset.
| strong: true, | ||
| em: true, | ||
| small: true, | ||
| sub: true, | ||
| sup: true, | ||
| u: true, | ||
| mark: true, | ||
| blockquote: true, | ||
| section: true, | ||
| article: true, | ||
| header: true, | ||
| footer: true, | ||
| main: true, | ||
| nav: true, | ||
| dl: true, | ||
| dt: true, | ||
| dd: true, | ||
| video: true, | ||
| audio: true, | ||
| picture: true, | ||
| source: true, | ||
| figure: true, | ||
| figcaption: true, | ||
| details: true, | ||
| summary: true, | ||
| fieldset: true, | ||
| legend: true, | ||
| progress: true, | ||
| meter: true, | ||
| }; |
There was a problem hiding this comment.
New tag whitelist expands attack surface; add URL-attribute sanitization.
We now allow media/navigation tags (img, video, audio, source, a, picture, figure, etc.) but props are passed through without URL/protocol validation. A malicious VDOM could set javascript: URLs or cross-origin src/href. Gate url-like attributes to http(s), data (images only), or relative paths; also default rel="noopener noreferrer" when target="_blank".
Apply this minimal guard:
@@
-function convertProps(elem: VDomElem, model: TsunamiModel): GenericPropsType {
+const UrlAttrs = new Set(["href","src","srcset","poster"]);
+function isSafeUrl(tag: string, attr: string, v: unknown): boolean {
+ if (typeof v !== "string") return true;
+ const val = v.trim();
+ if (val.startsWith("javascript:")) return false;
+ // allow relative
+ if (/^[./]/.test(val)) return true;
+ try {
+ const u = new URL(val, "http://x");
+ if (attr === "src" && tag === "img" && u.protocol === "data:") return true;
+ return u.protocol === "http:" || u.protocol === "https:";
+ } catch { return false; }
+}
+function convertProps(elem: VDomElem, model: TsunamiModel): GenericPropsType {
let props: GenericPropsType = {};
@@
- if (isObject(val) && val.type == VDomObjType_Func) {
+ if (isObject(val) && val.type == VDomObjType_Func) {
const valFunc = val as VDomFunc;
props[key] = convertVDomFunc(model, valFunc, elem.waveid, key);
continue;
}
- props[key] = val;
+ if (UrlAttrs.has(key) && !isSafeUrl(elem.tag, key, val)) {
+ continue; // drop unsafe URL
+ }
+ // Hardening: prevent tabnabbing
+ if (elem.tag === "a" && key === "target" && val === "_blank") {
+ props["rel"] = props["rel"] ? props["rel"] : "noopener noreferrer";
+ }
+ props[key] = val;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tsunami/frontend/src/vdom.tsx around lines 70-99 the tag whitelist was
expanded to include media/navigation tags but no URL/protocol checks are applied
to props; implement a sanitizer that inspects URL-like attributes (at minimum:
src, href, srcset, poster, data-*, formaction) before passing props into the
VDOM and reject or strip values whose protocol is not http:, https:, or a
relative path (allow data: only for image tags like img and picture sources);
for srcset handle each URL entry. Also, when rendering <a> ensure that if target
=== "_blank" you default rel to "noopener noreferrer" unless an explicit safe
rel is provided. Apply these checks where props are normalized/forwarded
(centralize validation), return sanitized props, and add unit tests covering
javascript: and data: cases and target="_blank" rel behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (12)
tsunami/app/defaultclient.go (1)
96-105: Resolved prior flag.Parse/io.ReadAll concerns; env‑var + io.Copy approach LGTM.
This directly addresses the earlier feedback to avoid flag collisions and unbounded reads.Please verify no stale docs/flags remain. Suggested check:
#!/bin/bash # Find lingering CLI flag mentions or outdated names. rg -n -C2 -g '!**/node_modules/**' -g '!**/dist/**' \ 'close-on-stdin|TSUNAMI_CLOSE_ON_STDIN|TSUNAMI_CLOSEONSTDIN|flag\.Parse\('tsunami/engine/clientimpl.go (3)
46-47: Use value RWMutex instead of pointer Mutex for SSE channel map; enable read locking for broadcasts.Switch to a value sync.RWMutex and rename to SSEChannelsMu; this avoids heap allocation, is idiomatic, and lets broadcasts take a read lock to reduce contention with register/unregister.
- SSEChannels map[string]chan ssEvent // map of connectionId to SSE channel - SSEChannelsLock *sync.Mutex + SSEChannels map[string]chan ssEvent // connectionId -> SSE channel + SSEChannelsMu sync.RWMutex
66-73: Initializer: drop pointer mutex allocation after switching to RWMutex.With a value RWMutex, rely on zero-value and remove the explicit allocation line.
Lock: &sync.Mutex{}, DoneCh: make(chan struct{}), SSEChannels: make(map[string]chan ssEvent), - SSEChannelsLock: &sync.Mutex{}, UrlHandlerMux: http.NewServeMux(), ServerId: uuid.New().String(), RootElem: vdom.H(DefaultComponentName, nil),
238-258: Broadcast under read lock; optionally deep-copy payload to ensure immutability.Use RLock/RUnlock (after adopting RWMutex) to reduce contention. If callers might mutate event.Data after SendSSEvent returns, copy once before broadcasting.
-func (c *ClientImpl) SendSSEvent(event ssEvent) error { +func (c *ClientImpl) SendSSEvent(event ssEvent) error { if c.GetIsDone() { return fmt.Errorf("client is done") } - c.SSEChannelsLock.Lock() - defer c.SSEChannelsLock.Unlock() + c.SSEChannelsMu.RLock() + defer c.SSEChannelsMu.RUnlock() - // Send to all registered SSE channels - for _, ch := range c.SSEChannels { + // Send to all registered SSE channels + ev := event + if len(event.Data) > 0 { + ev.Data = append([]byte(nil), event.Data...) // make payload immutable across handlers + } + for _, ch := range c.SSEChannels { select { - case ch <- event: + case ch <- ev: // Successfully sent default: // silently drop (below is just for debugging). this wont happen in general // log.Printf("SSEvent channel is full for connection %s, skipping event", connectionId) } } return nil }tsunami/build/buildutil.go (1)
32-71: Good addition of the closer function to prevent resource leaksThe refactored
pathToFSfunction now properly returns a closer function that can be used to release resources when opening.tsappfiles. This addresses the previous file-descriptor leak issue.pkg/blockcontroller/blockcontroller.go (5)
264-274: Consider cleaning up stopped controllers
StopAllBlockControllersleaves stopped controllers in the registry which could accumulate memory over time. Consider removing them after stopping.func StopAllBlockControllers() { controllers := getAllControllers() for blockId, controller := range controllers { status := controller.GetRuntimeStatus() if status != nil && status.ShellProcStatus == Status_Running { go func(id string, c Controller) { c.Stop(true, Status_Done) + deleteController(id) }(blockId, controller) } } }
84-98: Fix deadlock risk in registerControllerCalling
existingController.Stop(...)after releasing the lock is good, but there's still a race condition - another goroutine could register a different controller between releasing the lock and calling Stop, causing the wrong controller to be stopped.Apply this safer approach that minimizes the race window:
func registerController(blockId string, controller Controller) { var existingController Controller registryLock.Lock() existing, exists := controllerRegistry[blockId] if exists { existingController = existing } controllerRegistry[blockId] = controller registryLock.Unlock() if existingController != nil { - existingController.Stop(false, Status_Done) + // Stop with a small delay to ensure new controller is registered + go func() { + time.Sleep(10 * time.Millisecond) + existingController.Stop(false, Status_Done) + }() } }
161-167: Remove arbitrary sleep after StopThe 100ms sleep after
StopBlockControlleris brittle and can cause unnecessary delays. Either make Stop synchronous or implement proper wait mechanisms.if needsReplace { log.Printf("stopping blockcontroller %s due to controller type change\n", blockId) StopBlockController(blockId) - time.Sleep(100 * time.Millisecond) deleteController(blockId) existing = nil }
169-179: Remove sleep on connection changeSame issue - avoid arbitrary sleeps after stop operations.
if existingStatus.ShellProcStatus == Status_Running && existingStatus.ShellProcConnName != connName { log.Printf("stopping blockcontroller %s due to conn change\n", blockId) StopBlockControllerAndSetStatus(blockId, Status_Init) - time.Sleep(100 * time.Millisecond) // Don't delete, will reuse same controller type existing = getController(blockId) }
182-186: Remove sleep on force restartConsistent with other comments - avoid arbitrary sleeps.
if force && existing != nil { StopBlockController(blockId) - time.Sleep(100 * time.Millisecond) existing = getController(blockId) }pkg/blockcontroller/tsunamicontroller.go (2)
103-128: Build cache validation only checks app.go modification timeThe cache freshness check uses
build.GetAppModTimewhich only compares the cache modification time againstapp.go(or the.tsappfile itself), missing changes to other.gofiles or static assets. This could lead to stale binaries being used when only supporting files change.#!/bin/bash # Check GetAppModTime implementation to confirm it only checks app.go ast-grep --pattern 'func GetAppModTime($_, $_) { $$$ }'
373-417: Fix goroutine leak in port detectionThe stderr reader goroutine can leak if the context times out or the process dies - it tries to send on
errChanwhich has a buffer of 1, but if another goroutine has already sent, this will block forever. Also,io.EOFfromReadLinewill report as "stderr buffer error: EOF" instead of a more actionable message.Apply this diff to fix the goroutine leak and improve error messages:
// Monitor stderr for port information portChan := make(chan int, 1) errChan := make(chan error, 1) go func() { for { line, err := stderrBuffer.ReadLine() if err != nil { - errChan <- fmt.Errorf("stderr buffer error: %w", err) + // Use non-blocking send to avoid goroutine leak + select { + case errChan <- fmt.Errorf("tsunami stderr closed before listening message (EOF)"): + default: + } return } + // Log early stderr lines for debugging + log.Printf("[tsunami:%s] %s\n", appName, line) port := build.ParseTsunamiPort(line) if port > 0 { - portChan <- port + // Use non-blocking send to avoid leak + select { + case portChan <- port: + default: + } return } } }() // Wait for either port detection, process death, or context timeout go func() { <-tsunamiProc.WaitCh select { case <-portChan: // Port already found, nothing to do default: - errChan <- fmt.Errorf("tsunami process died before emitting listening message") + // Use non-blocking send to avoid goroutine leak + select { + case errChan <- fmt.Errorf("tsunami process died before emitting listening message"): + default: + } } }()
🧹 Nitpick comments (3)
tsunami/app/defaultclient.go (1)
18-18: Nit: prefer underscore in env var; optionally accept both names for transition.
Readability and convention usually favor TSUNAMI_CLOSE_ON_STDIN. If you want to keep today’s name during the PR, consider supporting both and deprecating the legacy one.Apply this diff:
-const TsunamiCloseOnStdinEnvVar = "TSUNAMI_CLOSEONSTDIN" +const TsunamiCloseOnStdinEnvVar = "TSUNAMI_CLOSE_ON_STDIN" +// Temporary back‑compat; remove before release if unused. +const TsunamiCloseOnStdinEnvVarLegacy = "TSUNAMI_CLOSEONSTDIN"And in RunMain:
- closeOnStdin := os.Getenv(TsunamiCloseOnStdinEnvVar) != "" + closeOnStdin := os.Getenv(TsunamiCloseOnStdinEnvVar) != "" || + os.Getenv(TsunamiCloseOnStdinEnvVarLegacy) != ""tsunami/engine/clientimpl.go (1)
260-263: Nit: remove newline in log.Printf.Avoid embedded newline; log.Printf appends one.
- log.Printf("send async initiation\n") + log.Printf("send async initiation")tsunami/build/buildutil.go (1)
114-146: Consider using copyFile instead of reimplementing file copy logicThe
copyFilefunction duplicates the exact same logic asCopyFileFromFSbut for local files. Consider refactoring to reuse the FS-based implementation.func copyFile(srcPath, destPath string) error { - // Get source file info for mode - srcInfo, err := os.Stat(srcPath) - if err != nil { - return err - } - - // Create destination directory if it doesn't exist - destDir := filepath.Dir(destPath) - if err := os.MkdirAll(destDir, 0755); err != nil { - return err - } - - srcFile, err := os.Open(srcPath) - if err != nil { - return err - } - defer srcFile.Close() - - destFile, err := os.Create(destPath) - if err != nil { - return err - } - defer destFile.Close() - - _, err = io.Copy(destFile, srcFile) - if err != nil { - return err - } - - // Set the same mode as source file - return os.Chmod(destPath, srcInfo.Mode()) + return CopyFileFromFS(os.DirFS("/"), strings.TrimPrefix(srcPath, "/"), destPath) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/blockcontroller/blockcontroller.go(3 hunks)pkg/blockcontroller/tsunamicontroller.go(1 hunks)pkg/utilds/readerlinebuffer.go(1 hunks)tsunami/app/defaultclient.go(2 hunks)tsunami/build/build.go(22 hunks)tsunami/build/buildutil.go(5 hunks)tsunami/engine/clientimpl.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tsunami/build/build.go
- pkg/utilds/readerlinebuffer.go
🧰 Additional context used
🧬 Code graph analysis (3)
tsunami/engine/clientimpl.go (2)
tsunami/engine/rootelem.go (1)
RootElem(36-49)tsunami/app/defaultclient.go (1)
SendAsyncInitiation(43-45)
pkg/blockcontroller/blockcontroller.go (10)
pkg/waveobj/wtype.go (4)
TermSize(304-307)RuntimeOpts(264-267)Block(279-287)Block(289-291)pkg/wstore/wstore_dbops.go (1)
DBMustGet(138-149)pkg/waveobj/metaconsts.go (2)
MetaKey_Controller(11-11)MetaKey_Connection(19-19)pkg/blockcontroller/shellcontroller.go (2)
ShellController(47-63)MakeShellController(66-75)pkg/blockcontroller/tsunamicontroller.go (2)
TsunamiController(36-46)MakeTsunamiController(420-427)pkg/filestore/blockstore.go (1)
WFS(49-52)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (5)
WaveEvent(21-27)Event_BlockFile(14-14)WSFileEventData(47-52)FileOp_Append(42-42)FileOp_Truncate(43-43)pkg/wavebase/wavebase.go (2)
BlockFile_Term(42-42)BlockFile_Cache(43-43)pkg/blocklogger/blocklogger.go (1)
Infof(78-84)
pkg/blockcontroller/tsunamicontroller.go (7)
pkg/utilds/readerlinebuffer.go (2)
ReaderLineBuffer(9-18)MakeReaderLineBuffer(20-35)pkg/wavebase/wavebase.go (5)
IsDevMode(106-108)GetHomeDir(130-136)TryMkdirs(207-223)ExpandHomeDir(138-152)GetWaveAppElectronExecPath(126-128)tsunami/build/build.go (5)
GetAppName(44-47)GetAppModTime(287-302)BuildOpts(32-42)TsunamiBuild(401-409)ParseTsunamiPort(819-842)pkg/blockcontroller/blockcontroller.go (7)
Status_Running(34-34)Status_Done(35-35)BlockControllerRuntimeStatus(54-61)SendInput(256-262)BlockInputUnion(48-52)Controller(64-69)Status_Init(36-36)pkg/wps/wps.go (1)
Broker(48-52)pkg/wps/wpstypes.go (2)
WaveEvent(21-27)Event_ControllerStatus(12-12)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)
⏰ 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: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (7)
tsunami/app/defaultclient.go (1)
6-13: LGTM: imports added for env‑var + stdin drain are correct and necessary.tsunami/engine/clientimpl.go (2)
228-236: Unregister logic looks good.Close-then-delete under the mutex prevents concurrent sends and avoids double-close.
219-226: Incorrect — no duplicate-registration leak (registration is unique and unregistered).
RegisterSSEChannel is only called in tsunami/engine/serverhandlers.go where connectionId is built as fmt.Sprintf("%s-%d", clientId, time.Now().UnixNano()) and the caller defers UnregisterSSEChannel(connectionId) (serverhandlers.go:347–348). No other usages found; the overwrite-without-close leak described is not present.Likely an incorrect or invalid review comment.
tsunami/build/buildutil.go (1)
285-287: Good practice: always using standard permissions for extracted directoriesThe comment correctly explains why 0755 permissions are used regardless of source permissions, which is important when extracting from zip files that may have read-only directories.
pkg/blockcontroller/tsunamicontroller.go (2)
130-249: Well-structured tsunami app lifecycle managementThe Start method properly handles scaffolding validation, build caching, and process lifecycle with appropriate status updates and goroutine coordination.
48-82: Platform-specific cache directory implementation looks correctThe
getCachesDirfunction properly follows platform conventions for cache directories on macOS, Linux, and Windows, with appropriate fallback to temp directory.pkg/blockcontroller/blockcontroller.go (1)
63-69: Well-designed Controller interfaceThe Controller interface provides a clean abstraction for different block controller types with appropriate lifecycle methods.
No description provided.