tsunami framework (waveapps v2) #2315
Conversation
WalkthroughAdds a new Tsunami framework and app stack. Backend: new VDOM types and RPC wire types, a three-pattern rendering engine, hooks and per-component hook state, atom and client implementations, root state manager, component shadow-tree model, async initiation coalescing, HTTP server handlers, build utilities and CLI, templates, and many utility packages (marshal, compare, util). Frontend: a Vite + React + TypeScript app (TsunamiModel, VDomView renderer, Recharts adapter, Markdown, OptimisticInput, key/platform utilities, types, Tailwind theme, model utils). Also adds many demo apps, Tailwind static assets, Taskfile tasks, package/workspace changes, editor settings, and scaffolding templates. Estimated code review effort🎯 5 (Critical) | ⏱️ ~240 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
tsunami/app/hooks.go (3)
50-58: Add missing nil-context guard to UseVDomRef (prevents NPE).Calling engine.UseVDomRef with a nil context will panic inside the engine. Guard like other hooks.
Apply:
func UseVDomRef() *vdom.VDomRef { - rc := engine.GetGlobalContext() - val := engine.UseVDomRef(rc) + rc := engine.GetGlobalContext() + if rc == nil { + panic("UseVDomRef must be called within a component (no context)") + } + val := engine.UseVDomRef(rc) refVal, ok := val.(*vdom.VDomRef) if !ok { panic("UseVDomRef hook value is not a ref (possible out of order or conditional hooks)") } return refVal }
64-72: Add missing nil-context guard to UseRef.Mirror other hooks for consistent, early failure.
Apply:
func UseRef[T any](val T) *vdom.VDomSimpleRef[T] { - rc := engine.GetGlobalContext() + rc := engine.GetGlobalContext() + if rc == nil { + panic("UseRef must be called within a component (no context)") + } refVal := engine.UseRef(rc, &vdom.VDomSimpleRef[T]{Current: val}) typedRef, ok := refVal.(*vdom.VDomSimpleRef[T]) if !ok { panic("UseRef hook value is not a ref (possible out of order or conditional hooks)") } return typedRef }
132-146: Guard against nil default client in UseLocal.If the default client isn’t initialized, Atom ops will NPE later. Fail fast here.
Apply:
atomName := engine.UseLocal(rc, initialVal) - return Atom[T]{ - name: atomName, - client: engine.GetDefaultClient(), - } + client := engine.GetDefaultClient() + if client == nil { + panic("UseLocal: default client is not initialized") + } + return Atom[T]{name: atomName, client: client}tsunami/engine/render.go (1)
201-232: Fix: interface{} and pointer arg handling in callCFunc; stop print-and-return nil (panic is caught by guard).Components taking
anycurrently receive nil, and pointer-struct signatures risk passing a ptr-to-ptr into MapToStruct. Also, invalid arity/prop conversion merely prints and returns nil, silently breaking UI. Let callCFunc build the correct argument and panic on contract violations so callCFuncWithErrorGuard can surface an error component.func callCFunc(cfunc any, props map[string]any) any { rval := reflect.ValueOf(cfunc) rtype := rval.Type() - if rtype.NumIn() != 1 { - fmt.Printf("component function must have exactly 1 parameter, got %d\n", rtype.NumIn()) - return nil - } + if rtype.NumIn() != 1 { + panic(fmt.Errorf("component function must have exactly 1 parameter, got %d", rtype.NumIn())) + } - argType := rtype.In(0) - - var arg1Val reflect.Value - if argType.Kind() == reflect.Interface && argType.NumMethod() == 0 { - arg1Val = reflect.New(argType) - } else { - arg1Val = reflect.New(argType) - if argType.Kind() == reflect.Map { - arg1Val.Elem().Set(reflect.ValueOf(props)) - } else { - err := util.MapToStruct(props, arg1Val.Interface()) - if err != nil { - fmt.Printf("error converting props: %v\n", err) - } - } - } - rtnVal := rval.Call([]reflect.Value{arg1Val.Elem()}) - if len(rtnVal) == 0 { - return nil - } - return rtnVal[0].Interface() + argType := rtype.In(0) + var callArg reflect.Value + switch { + case argType.Kind() == reflect.Interface && argType.NumMethod() == 0: + // interface{} → pass props directly + callArg = reflect.ValueOf(props) + case argType.Kind() == reflect.Map: + // map[string]any → pass props directly + callArg = reflect.ValueOf(props) + default: + // struct or *struct + isPtr := argType.Kind() == reflect.Ptr + targetType := argType + if isPtr { + targetType = argType.Elem() + } + target := reflect.New(targetType) + if err := util.MapToStruct(props, target.Interface()); err != nil { + panic(fmt.Errorf("converting props for component: %w", err)) + } + if isPtr { + callArg = target + } else { + callArg = target.Elem() + } + } + rtnVal := rval.Call([]reflect.Value{callArg}) + if len(rtnVal) == 0 { + panic(fmt.Errorf("component returned no value")) + } + return rtnVal[0].Interface() }tsunami/engine/rootelem.go (5)
180-184: Typo in error string.“cannot b nil” → “cannot be nil”.
- if cfunc == nil { - return fmt.Errorf("Component function cannot b nil") - } + if cfunc == nil { + return fmt.Errorf("Component function cannot be nil") + }
266-296: Guard effect indexes and nil root to prevent panics in RunWork.Out-of-range
EffectIndexor nil hooks can panic;r.Root/r.Root.Elemmay be nil on first run.// first, run effect cleanups for _, work := range workQueue { comp := r.CompMap[work.Id] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - if hook.UnmountFn != nil { + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + if hook.UnmountFn != nil { hook.UnmountFn() } } // now run, new effects for _, work := range workQueue { comp := r.CompMap[work.Id] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - if hook.Fn != nil { + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + if hook.Fn != nil { hook.UnmountFn = hook.Fn() } } // now check if we need a render if len(r.NeedsRenderMap) > 0 { r.NeedsRenderMap = nil - r.render(r.Root.Elem, &r.Root, "root", opts) + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } }
151-171: Avoid lock coupling: release atomLock before calling atom methods.Holding
atomLockwhile invokingSetVal/SetFnValrisks deadlocks and stalls if atom methods lock internally or execute user code.func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) } func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetFnVal(setFn) }
34-45: Protect RefOperations with a mutex to avoid races.
QueueRefOp/GetRefOperationsmutate shared state without synchronization.type RootElem struct { Root *ComponentImpl RenderTs int64 AppTitle string CFuncs map[string]any CompMap map[string]*ComponentImpl // component waveid -> component EffectWorkQueue []*EffectWorkElem NeedsRenderMap map[string]bool Atoms map[string]genAtom atomLock sync.Mutex RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex }func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }Also applies to: 331-339
212-218: Reject duplicate component registrations.Silently overwriting components is dangerous.
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if r.CFuncs == nil { + r.CFuncs = make(map[string]any) + } + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }
🧹 Nitpick comments (6)
tsunami/app/hooks.go (3)
74-78: Doc nit: ID is not guaranteed to be a UUID.Engine returns a “wave id”; adjust wording to avoid implying UUID.
Apply:
-// UseId returns the underlying component's unique identifier (UUID). +// UseId returns the component's unique identifier (wave/component id).
151-189: Tighten cleanup in UseGoRoutine (clear ref after cancel).Minor leak/clarity improvement: reset the cancel ref after canceling.
Apply:
// Cancel any existing goroutine if cancelRef.Current != nil { cancelRef.Current() + cancelRef.Current = nil } @@ // Return cleanup function that cancels the context return func() { if cancel != nil { cancel() } + // clear the stored cancel func + cancelRef.Current = nil }
215-227: Optional: clarify semantics for non-positive durations in UseAfter.Go timers fire immediately for <=0 durations; consider making this explicit for readability.
Apply (one option):
func UseAfter(duration time.Duration, timeoutFn func(), deps []any) { - UseGoRoutine(func(ctx context.Context) { + if duration <= 0 { + // Run immediately in a lifecycle-managed goroutine for consistency + UseGoRoutine(func(ctx context.Context) { timeoutFn() }, deps) + return + } + UseGoRoutine(func(ctx context.Context) { timer := time.NewTimer(duration) defer timer.Stop()tsunami/engine/render.go (2)
90-94: Prefer indexing over taking the address of the range variable when unmounting children.Passing
&childtakes the address of the loop variable, not the slice element. It works here but is non-idiomatic and fragile.- for _, child := range (*comp).Children { - r.unmount(&child) - } + for i := range (*comp).Children { + r.unmount(&((*comp).Children[i])) + }- if (*comp).Children != nil { - for _, child := range (*comp).Children { - r.unmount(&child) - } - } + if (*comp).Children != nil { + for i := range (*comp).Children { + r.unmount(&((*comp).Children[i])) + } + }Also applies to: 129-133
165-168: Avoid inserting nil keys into usedMap.Guard
curChildto keepusedMapclean and intent clear.- usedMap[curChild] = true + if curChild != nil { + usedMap[curChild] = true + }tsunami/engine/rootelem.go (1)
220-244: callVDomFn: support VDomFunc values (not just pointers).Events won’t fire if props hold a value
vdom.VDomFunc.func callVDomFn(fnVal any, data vdom.VDomEvent) { if fnVal == nil { return } - fn := fnVal - if vdf, ok := fnVal.(*vdom.VDomFunc); ok { - fn = vdf.Fn - } + var fn any + switch t := fnVal.(type) { + case *vdom.VDomFunc: + fn = t.Fn + case vdom.VDomFunc: + fn = t.Fn + default: + fn = fnVal + } if fn == nil { return } rval := reflect.ValueOf(fn) if rval.Kind() != reflect.Func { return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tsunami/app/hooks.go(1 hunks)tsunami/engine/render.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tsunami/engine/render.go (6)
tsunami/engine/rootelem.go (2)
RootElem(34-45)ChildrenPropKey(19-19)tsunami/vdom/vdom_types.go (8)
VDomElem(17-22)KeyPropKey(11-11)TextTag(6-6)FragmentTag(9-9)VDomFunc(25-32)ObjectType_Func(14-14)VDomRef(35-41)ObjectType_Ref(13-13)tsunami/engine/comp.go (2)
ComponentImpl(21-42)ChildKey(11-15)tsunami/vdom/vdom.go (1)
ToElems(147-178)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/rpctypes/protocoltypes.go (1)
RenderedElem(13-19)
tsunami/app/hooks.go (5)
tsunami/engine/hooks.go (9)
UseState(68-86)UseVDomRef(135-147)UseRef(149-156)UseId(64-66)UseRenderTs(60-62)UseResync(195-200)UseEffect(170-193)UseSetAppTitle(202-208)UseLocal(120-133)tsunami/engine/globalctx.go (1)
GetGlobalContext(38-46)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)VDomSimpleRef(43-45)tsunami/engine/clientimpl.go (1)
GetDefaultClient(67-69)tsunami/util/util.go (1)
PanicHandler(24-34)
tsunami/engine/rootelem.go (6)
tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
tsunami/app/hooks.go (2)
16-43: LGTM: Typed UseState wrapper is solidCovers context guard, type assertion, and typed setters cleanly.
108-121: LGTM: UseEffect wrapper mirrors engine semantics.Nil-deps => always run; guarded context; queues effects only.
| func UseTicker(interval time.Duration, tickFn func(), deps []any) { | ||
| UseGoRoutine(func(ctx context.Context) { | ||
| ticker := time.NewTicker(interval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| tickFn() | ||
| } | ||
| } | ||
| }, deps) | ||
| } |
There was a problem hiding this comment.
Validate interval in UseTicker to avoid time.NewTicker panic.
time.NewTicker panics if interval <= 0. Check and fail early with a clear message.
Apply:
func UseTicker(interval time.Duration, tickFn func(), deps []any) {
- UseGoRoutine(func(ctx context.Context) {
+ if interval <= 0 {
+ panic("UseTicker: interval must be > 0")
+ }
+ UseGoRoutine(func(ctx context.Context) {
ticker := time.NewTicker(interval)
defer ticker.Stop()📝 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 UseTicker(interval time.Duration, tickFn func(), deps []any) { | |
| UseGoRoutine(func(ctx context.Context) { | |
| ticker := time.NewTicker(interval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| tickFn() | |
| } | |
| } | |
| }, deps) | |
| } | |
| func UseTicker(interval time.Duration, tickFn func(), deps []any) { | |
| if interval <= 0 { | |
| panic("UseTicker: interval must be > 0") | |
| } | |
| UseGoRoutine(func(ctx context.Context) { | |
| ticker := time.NewTicker(interval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| tickFn() | |
| } | |
| } | |
| }, deps) | |
| } |
🤖 Prompt for AI Agents
In tsunami/app/hooks.go around lines 195 to 209, UseTicker currently calls
time.NewTicker without validating interval which will panic if interval <= 0;
add an explicit check at the start of UseTicker that validates interval > 0 and
fail early with a clear message (e.g., panic or log+return) instead of letting
time.NewTicker panic, so callers get a deterministic, descriptive error before
starting the goroutine.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
tsunami/engine/globalctx.go (1)
15-21: Global singleton context will break under concurrent or re-entrant renders; scope by goroutine (RWMutex + per-GID maps).A single slot for render/event contexts means goroutines race and overwrite state; nested set/clear on the same goroutine also loses the outer context. Use a per-goroutine map with RLock in getters. If re-entrancy on the same goroutine is possible, upgrade to a stack.
Apply:
@@ -var globalRenderContext *RenderContextImpl -var globalRenderGoId uint64 - -var globalEventContext *EventContextImpl -var globalEventGoId uint64 - -var globalCtxMutex sync.Mutex +var globalCtxMutex sync.RWMutex +var globalRenderCtxByGID = map[uint64]*RenderContextImpl{} +var globalEventCtxByGID = map[uint64]*EventContextImpl{} @@ func setGlobalRenderContext(vc *RenderContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = vc - globalRenderGoId = goid.Get() + gid := goid.Get() + globalCtxMutex.Lock() + globalRenderCtxByGID[gid] = vc + globalCtxMutex.Unlock() } @@ func clearGlobalRenderContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = nil - globalRenderGoId = 0 + gid := goid.Get() + globalCtxMutex.Lock() + delete(globalRenderCtxByGID, gid) + globalCtxMutex.Unlock() } @@ func GetGlobalRenderContext() *RenderContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalRenderGoId { - return nil - } - return globalRenderContext + gid := goid.Get() + globalCtxMutex.RLock() + vc := globalRenderCtxByGID[gid] + globalCtxMutex.RUnlock() + return vc } @@ func setGlobalEventContext(ec *EventContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEventContext = ec - globalEventGoId = goid.Get() + gid := goid.Get() + globalCtxMutex.Lock() + globalEventCtxByGID[gid] = ec + globalCtxMutex.Unlock() } @@ func clearGlobalEventContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEventContext = nil - globalEventGoId = 0 + gid := goid.Get() + globalCtxMutex.Lock() + delete(globalEventCtxByGID, gid) + globalCtxMutex.Unlock() } @@ func GetGlobalEventContext() *EventContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalEventGoId { - return nil - } - return globalEventContext + gid := goid.Get() + globalCtxMutex.RLock() + ec := globalEventCtxByGID[gid] + globalCtxMutex.RUnlock() + return ec }Optional (if nested renders/events on the same goroutine can happen): use
map[uint64][]*RenderContextImpland push/pop in set/clear.Also applies to: 27-33, 34-39, 47-55, 57-62, 64-69, 77-85
tsunami/app/atom.go (2)
35-39: sameRef: pointer nils contradict the comment; guard nil before equality.Two nil pointers currently return true; comment says “Nil values return false.”
case reflect.Ptr: - // direct comparison works for *T - return any(oldVal) == any(newVal) + // honor "nil values return false" + if vOld.IsZero() || vNew.IsZero() { + return false + } + // direct comparison works for *T + return any(oldVal) == any(newVal)
110-129: SetFn promises a safe copy but passes the live value; deep-copy before calling user fn.As written, maps/slices/pointers can be mutated in-place before commit.
func (a Atom[T]) SetFn(fn func(T) T) { @@ - err := a.client.Root.SetFnAtomVal(a.name, func(val any) any { - typedVal := util.GetTypedAtomValue[T](val, a.name) - return fn(typedVal) - }) + err := a.client.Root.SetFnAtomVal(a.name, func(val any) any { + typedVal := util.GetTypedAtomValue[T](val, a.name) + // ensure user code mutates a copy, not live state + copied := DeepCopy(typedVal) + result := fn(copied) + if sameRef(typedVal, result) { + logMutationWarning(a.name) + } + return result + })If
DeepCopyisn’t available inapp, either add it or remove the “can be safely mutated” claim from the comment.tsunami/app/hooks.go (4)
51-58: Add nil-context guard in UseVDomRef to prevent NPE.Without the check, engine.UseVDomRef(rc) will deref a nil vc and panic with an obscure message.
Apply:
func UseVDomRef() *vdom.VDomRef { rc := engine.GetGlobalRenderContext() - val := engine.UseVDomRef(rc) + if rc == nil { + panic("UseVDomRef must be called within a component (no context)") + } + val := engine.UseVDomRef(rc) refVal, ok := val.(*vdom.VDomRef) if !ok { panic("UseVDomRef hook value is not a ref (possible out of order or conditional hooks)") } return refVal }
65-72: Add nil-context guard in UseRef to prevent NPE.Mirror other hooks for consistent failure mode.
Apply:
func UseRef[T any](val T) *vdom.VDomSimpleRef[T] { rc := engine.GetGlobalRenderContext() + if rc == nil { + panic("UseRef must be called within a component (no context)") + } refVal := engine.UseRef(rc, &vdom.VDomSimpleRef[T]{Current: val}) typedRef, ok := refVal.(*vdom.VDomSimpleRef[T]) if !ok { panic("UseRef hook value is not a ref (possible out of order or conditional hooks)") } return typedRef }
140-145: Guard against nil default client in UseLocal.Avoids later nil deref when Atom methods use the client.
Apply:
func UseLocal[T any](initialVal T) Atom[T] { rc := engine.GetGlobalRenderContext() if rc == nil { panic("UseLocal must be called within a component (no context)") } atomName := engine.UseLocal(rc, initialVal) - return Atom[T]{ - name: atomName, - client: engine.GetDefaultClient(), - } + client := engine.GetDefaultClient() + if client == nil { + panic("UseLocal: default client is not initialized") + } + return Atom[T]{name: atomName, client: client} }
195-209: Validate interval in UseTicker; time.NewTicker panics on <= 0.Fail fast with a clear message.
Apply:
func UseTicker(interval time.Duration, tickFn func(), deps []any) { + if interval <= 0 { + panic("UseTicker: interval must be > 0") + } UseGoRoutine(func(ctx context.Context) { ticker := time.NewTicker(interval) defer ticker.Stop()tsunami/engine/rootelem.go (6)
292-301: Guard hook index and nil hooks in RunWork to avoid panics.Indexing comp.Hooks without range/nil checks can panic during effect cleanup/init.
Apply:
for _, work := range workQueue { comp := r.CompMap[work.Id] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - if hook.UnmountFn != nil { - hook.UnmountFn() - } + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + if hook.UnmountFn != nil { + hook.UnmountFn() + } } // now run, new effects for _, work := range workQueue { comp := r.CompMap[work.Id] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - if hook.Fn != nil { - hook.UnmountFn = hook.Fn() - } + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + if hook.Fn != nil { + hook.UnmountFn = hook.Fn() + } }Also applies to: 303-312
314-317: Nil-guard root before render to avoid first-run NPE.RunWork can be called before root is initialized.
Apply:
if len(r.NeedsRenderMap) > 0 { r.NeedsRenderMap = nil - r.render(r.Root.Elem, &r.Root, "root", opts) + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } }
353-361: Protect RefOperations with a mutex to prevent data races.These methods may be called concurrently (e.g., from default client).
Apply:
type RootElem struct { Root *ComponentImpl RenderTs int64 AppTitle string CFuncs map[string]any CompMap map[string]*ComponentImpl // component waveid -> component EffectWorkQueue []*EffectWorkElem NeedsRenderMap map[string]bool Atoms map[string]genAtom atomLock sync.Mutex RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex }func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }
151-160: Avoid holding atomLock while calling atom.SetVal/SetFnVal (lock coupling).Release r.atomLock after lookup to reduce deadlock risk and stalls.
Apply:
func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) } func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetFnVal(setFn) }Also applies to: 162-171
212-218: Reject duplicate component registration.Overwriting silently can cause hard-to-debug behavior.
Apply:
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }
180-183: Fix typo in error message: “cannot b nil” → “cannot be nil”.User-facing string.
Apply:
- if cfunc == nil { - return fmt.Errorf("Component function cannot b nil") - } + if cfunc == nil { + return fmt.Errorf("Component function cannot be nil") + }
🧹 Nitpick comments (2)
tsunami/app/hooks.go (1)
215-227: Optional: consider guarding negative durations in UseAfter.Negative durations fire immediately; if that’s undesired, validate duration > 0 or document behavior.
tsunami/engine/rootelem.go (1)
246-284: Optional: consider synchronization strategy for CompMap access during events.If events can fire concurrently with render, reads of r.CompMap/r.Comp fields may race. Either serialize on a single engine goroutine or add RW locking around CompMap mutations/reads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tsunami/app/atom.go(1 hunks)tsunami/app/hooks.go(1 hunks)tsunami/app/root_test.go(1 hunks)tsunami/engine/clientimpl.go(1 hunks)tsunami/engine/globalctx.go(1 hunks)tsunami/engine/hooks.go(1 hunks)tsunami/engine/render.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)tsunami/engine/serverhandlers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tsunami/engine/serverhandlers.go
- tsunami/app/root_test.go
- tsunami/engine/render.go
- tsunami/engine/clientimpl.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-23T06:09:05.334Z
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1807
File: pkg/wshrpc/wshremote/wshremote.go:718-726
Timestamp: 2025-01-23T06:09:05.334Z
Learning: In Go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.Field != nil`) is the idiomatic way to handle multiple nil checks. Avoid suggesting nested if statements as they make the code more verbose without adding value.
Applied to files:
tsunami/app/atom.go
🧬 Code graph analysis (5)
tsunami/app/hooks.go (6)
tsunami/engine/hooks.go (9)
UseState(68-86)UseVDomRef(135-147)UseRef(149-156)UseId(64-66)UseRenderTs(60-62)UseResync(195-200)UseEffect(170-193)UseSetAppTitle(202-208)UseLocal(120-133)tsunami/engine/globalctx.go (1)
GetGlobalRenderContext(47-55)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)VDomSimpleRef(43-45)tsunami/app/atom.go (1)
Atom(62-65)tsunami/engine/clientimpl.go (1)
GetDefaultClient(67-69)tsunami/util/util.go (1)
PanicHandler(24-34)
tsunami/engine/hooks.go (6)
tsunami/engine/rootelem.go (1)
RootElem(34-45)tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/app/hooks.go (8)
UseRenderTs(88-94)UseId(78-84)UseState(21-43)UseVDomRef(50-58)UseRef(64-72)UseEffect(113-120)UseResync(100-106)UseSetAppTitle(124-130)tsunami/engine/atomimpl.go (1)
MakeAtomImpl(18-24)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)ObjectType_Ref(13-13)
tsunami/app/atom.go (3)
tsunami/engine/clientimpl.go (1)
ClientImpl(36-52)tsunami/engine/globalctx.go (1)
GetGlobalRenderContext(47-55)tsunami/util/util.go (1)
GetTypedAtomValue(103-121)
tsunami/engine/globalctx.go (2)
tsunami/engine/hooks.go (1)
RenderContextImpl(23-28)tsunami/vdom/vdom_types.go (1)
VDomEvent(65-75)
tsunami/engine/rootelem.go (7)
tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/globalctx.go (1)
EventContextImpl(23-25)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
| import ( | ||
| "log" | ||
| "strconv" | ||
|
|
||
| "github.com/wavetermdev/waveterm/tsunami/vdom" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import reflect for safe dependency comparisons.
Required by the depsEqual fix below.
import (
"log"
"strconv"
+ "reflect"
"github.com/wavetermdev/waveterm/tsunami/vdom"
)📝 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.
| import ( | |
| "log" | |
| "strconv" | |
| "github.com/wavetermdev/waveterm/tsunami/vdom" | |
| ) | |
| import ( | |
| "log" | |
| "strconv" | |
| "reflect" | |
| "github.com/wavetermdev/waveterm/tsunami/vdom" | |
| ) |
🤖 Prompt for AI Agents
In tsunami/engine/hooks.go around lines 6 to 11, the import list is missing the
reflect package required by the upcoming depsEqual change; add "reflect" to the
imported packages (alongside log, strconv, and the vdom import) so the code
compiles and can perform safe dependency comparisons using reflect.DeepEqual in
the depsEqual fix.
tsunami/engine/hooks.go
Outdated
| setFuncVal := func(updateFunc func(any) any) { | ||
| currentVal := vc.Root.GetAtomVal(atomName) | ||
| if err := vc.Root.SetAtomVal(atomName, updateFunc(currentVal)); err != nil { | ||
| log.Printf("Failed to set atom value for %s: %v", atomName, err) | ||
| return | ||
| } | ||
| vc.Root.AtomAddRenderWork(atomName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UseAtom setFuncVal should be atomic (RMW) to avoid lost updates.
Use the engine’s SetFnAtomVal instead of Get+Set.
setFuncVal := func(updateFunc func(any) any) {
- currentVal := vc.Root.GetAtomVal(atomName)
- if err := vc.Root.SetAtomVal(atomName, updateFunc(currentVal)); err != nil {
+ if err := vc.Root.SetFnAtomVal(atomName, updateFunc); err != nil {
log.Printf("Failed to set atom value for %s: %v", atomName, err)
return
}
vc.Root.AtomAddRenderWork(atomName)
}📝 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.
| setFuncVal := func(updateFunc func(any) any) { | |
| currentVal := vc.Root.GetAtomVal(atomName) | |
| if err := vc.Root.SetAtomVal(atomName, updateFunc(currentVal)); err != nil { | |
| log.Printf("Failed to set atom value for %s: %v", atomName, err) | |
| return | |
| } | |
| vc.Root.AtomAddRenderWork(atomName) | |
| } | |
| setFuncVal := func(updateFunc func(any) any) { | |
| if err := vc.Root.SetFnAtomVal(atomName, updateFunc); err != nil { | |
| log.Printf("Failed to set atom value for %s: %v", atomName, err) | |
| return | |
| } | |
| vc.Root.AtomAddRenderWork(atomName) | |
| } |
| func depsEqual(deps1 []any, deps2 []any) bool { | ||
| if len(deps1) != len(deps2) { | ||
| return false | ||
| } | ||
| for i := range deps1 { | ||
| if deps1[i] != deps2[i] { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
depsEqual can panic on uncomparable types (maps, slices, funcs); make comparison safe.
Current deps[i] != deps2[i] will panic for common dep shapes.
-func depsEqual(deps1 []any, deps2 []any) bool {
- if len(deps1) != len(deps2) {
- return false
- }
- for i := range deps1 {
- if deps1[i] != deps2[i] {
- return false
- }
- }
- return true
-}
+func depsEqual(deps1 []any, deps2 []any) bool {
+ if len(deps1) != len(deps2) {
+ return false
+ }
+ for i := range deps1 {
+ d1, d2 := deps1[i], deps2[i]
+ if (d1 == nil) != (d2 == nil) {
+ return false
+ }
+ if d1 == nil { // both nil
+ continue
+ }
+ t1, t2 := reflect.TypeOf(d1), reflect.TypeOf(d2)
+ if t1 != t2 {
+ return false
+ }
+ if t1.Comparable() {
+ // safe interface equality
+ if d1 != d2 {
+ return false
+ }
+ continue
+ }
+ v1, v2 := reflect.ValueOf(d1), reflect.ValueOf(d2)
+ switch v1.Kind() {
+ case reflect.Slice, reflect.Map, reflect.Chan, reflect.Func, reflect.Pointer:
+ // treat identity equality like React's referential deps semantics
+ if v1.IsNil() || v2.IsNil() {
+ if v1.IsNil() && v2.IsNil() {
+ continue
+ }
+ return false
+ }
+ if v1.Pointer() != v2.Pointer() {
+ return false
+ }
+ default:
+ // last resort: deep equal to avoid panics for non-comparable structs
+ if !reflect.DeepEqual(d1, d2) {
+ return false
+ }
+ }
+ }
+ return true
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tsunami/engine/hooks.go around lines 158 to 168, depsEqual currently does a
direct equality check (deps1[i] != deps2[i]) which will panic on uncomparable
types like maps, slices, or funcs; replace the direct comparison with a safe
reflect.DeepEqual-based comparison for each element (keep the length check),
ensuring you import the reflect package, and handle nil vs non-nil correctly by
relying on reflect.DeepEqual to return false for mismatches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
tsunami/engine/globalctx.go (2)
13-25: Global single-slot contexts are not goroutine-safe and are not re-entrancy-safe. Use per-goroutine stacks.Concurrent renders/events/effects on different goroutines will stomp each other, and nested calls on the same goroutine will clear the outer context. Switch to per-goroutine maps with stack semantics and RW locks.
Apply:
-// is set ONLY when we're in the render function of a component -// used for hooks, and automatic dependency tracking -var globalRenderContext *RenderContextImpl -var globalRenderGoId uint64 - -var globalEventContext *EventContextImpl -var globalEventGoId uint64 - -var globalEffectContext *EffectContextImpl -var globalEffectGoId uint64 - -var globalCtxMutex sync.Mutex +// Goroutine-scoped context stacks to support concurrency and re-entrancy. +var globalCtxMutex sync.RWMutex +var renderCtxByGID = map[uint64][]*RenderContextImpl{} +var eventCtxByGID = map[uint64][]*EventContextImpl{} +var effectCtxByGID = map[uint64][]*EffectContextImpl{}And update render context setters/getters:
-func setGlobalRenderContext(vc *RenderContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = vc - globalRenderGoId = goid.Get() -} +func setGlobalRenderContext(vc *RenderContextImpl) { + globalCtxMutex.Lock() + gid := goid.Get() + renderCtxByGID[gid] = append(renderCtxByGID[gid], vc) + globalCtxMutex.Unlock() +} -func clearGlobalRenderContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = nil - globalRenderGoId = 0 -} +func clearGlobalRenderContext() { + globalCtxMutex.Lock() + gid := goid.Get() + stack := renderCtxByGID[gid] + if n := len(stack); n > 0 { + stack = stack[:n-1] + if len(stack) == 0 { + delete(renderCtxByGID, gid) + } else { + renderCtxByGID[gid] = stack + } + } + globalCtxMutex.Unlock() +} -func GetGlobalRenderContext() *RenderContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalRenderGoId { - return nil - } - return globalRenderContext -} +func GetGlobalRenderContext() *RenderContextImpl { + gid := goid.Get() + globalCtxMutex.RLock() + stack := renderCtxByGID[gid] + var vc *RenderContextImpl + if n := len(stack); n > 0 { + vc = stack[n-1] + } + globalCtxMutex.RUnlock() + return vc +}Repeat the same stack pattern for event/effect contexts below (set/clear/get).
-/* event/effect: set/clear/get use single-slot */ +/* event/effect: set/clear/get use per-goroutine stacks */
51-55: Nested withGlobal*Ctx calls will clear outer context.Current push-then-clear semantics with single slots break on re-entrancy. After adopting stacks, the existing withGlobal*Ctx wrappers are fine (push/pop). Without the stack change, this is a correctness bug.
Also applies to: 81-85, 111-115
tsunami/engine/rootelem.go (6)
181-184: Fix typo in user-facing error.“cannot b nil” → “cannot be nil”.
Apply:
- return fmt.Errorf("Component function cannot b nil") + return fmt.Errorf("Component function cannot be nil")
396-404: Synchronize RefOperations to avoid data races.Both QueueRefOp and GetRefOperations mutate shared slice without locking. Add a mutex.
Apply:
type RootElem struct { @@ - RefOperations []vdom.VDomRefOperation + RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex } func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }Also applies to: 35-46
339-355: Guard effect hook access to prevent panics.Indexing comp.Hooks without bounds/nil checks can panic.
Apply:
for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } r.runEffectUnmount(work, hook) } // now run, new effects for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } r.runEffect(work, hook) }
357-360: Nil-guard root before render to avoid a nil deref.RunWork may run before first render.
Apply:
if len(r.NeedsRenderMap) > 0 { r.NeedsRenderMap = nil - r.render(r.Root.Elem, &r.Root, "root", opts) + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } }
152-172: Avoid lock coupling: don’t hold atomLock while calling atom methods.Release root lock after lookup to prevent deadlocks and stalls.
Apply:
func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) } func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetFnVal(setFn) }
213-219: Prevent accidental component re-registration.Reject duplicates instead of silently overwriting.
Apply:
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }
🧹 Nitpick comments (4)
tsunami/engine/globalctx.go (2)
57-65: Use RLock in getters to reduce contention.Minor: getters take the write lock. Switch to RLock/RUnlock (shown in the diff above) for read paths.
Also applies to: 87-95, 117-125
6-11: Consider passing contexts via context.Context instead of globals.Architecturally cleaner and test-friendly. Keep the global stack as a pragmatic step if API churn is a concern.
tsunami/engine/rootelem.go (2)
128-139: Also avoid holding atomLock while scheduling renders.Take atom, unlock, then iterate usedBy and call AddRenderWork outside the lock.
Apply:
func (r *RootElem) AtomAddRenderWork(atomName string) { - r.atomLock.Lock() - defer r.atomLock.Unlock() - atom, ok := r.Atoms[atomName] + r.atomLock.Lock() + atom, ok := r.Atoms[atomName] + r.atomLock.Unlock() if !ok { return } - for _, compId := range atom.GetUsedBy() { + usedBy := atom.GetUsedBy() + for _, compId := range usedBy { r.AddRenderWork(compId) } }
59-71: Reduce lock hold time in GetDataMap/GetConfigMap.Copy references under lock, then call GetVal without holding atomLock.
Apply:
func (r *RootElem) GetDataMap() map[string]any { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - result := make(map[string]any) - for atomName, atom := range r.Atoms { - if strings.HasPrefix(atomName, "$data.") { - strippedName := strings.TrimPrefix(atomName, "$data.") - result[strippedName] = atom.GetVal() - } - } - return result + r.atomLock.Lock() + type pair struct{ name string; a genAtom } + var pairs []pair + for atomName, atom := range r.Atoms { + if strings.HasPrefix(atomName, "$data.") { + pairs = append(pairs, pair{name: strings.TrimPrefix(atomName, "$data."), a: atom}) + } + } + r.atomLock.Unlock() + result := make(map[string]any, len(pairs)) + for _, p := range pairs { + result[p.name] = p.a.GetVal() + } + return result } func (r *RootElem) GetConfigMap() map[string]any { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - result := make(map[string]any) - for atomName, atom := range r.Atoms { - if strings.HasPrefix(atomName, "$config.") { - strippedName := strings.TrimPrefix(atomName, "$config.") - result[strippedName] = atom.GetVal() - } - } - return result + r.atomLock.Lock() + type pair struct{ name string; a genAtom } + var pairs []pair + for atomName, atom := range r.Atoms { + if strings.HasPrefix(atomName, "$config.") { + pairs = append(pairs, pair{name: strings.TrimPrefix(atomName, "$config."), a: atom}) + } + } + r.atomLock.Unlock() + result := make(map[string]any, len(pairs)) + for _, p := range pairs { + result[p.name] = p.a.GetVal() + } + return result }Also applies to: 73-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tsunami/engine/globalctx.go(1 hunks)tsunami/engine/hooks.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsunami/engine/hooks.go
🧰 Additional context used
🧬 Code graph analysis (2)
tsunami/engine/globalctx.go (3)
tsunami/engine/hooks.go (1)
RenderContextImpl(23-28)tsunami/vdom/vdom_types.go (1)
VDomEvent(65-75)tsunami/engine/rootelem.go (2)
RootElem(35-46)EffectWorkElem(21-25)
tsunami/engine/rootelem.go (8)
tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/globalctx.go (2)
EventContextImpl(26-29)EffectContextImpl(31-35)tsunami/engine/hooks.go (1)
Hook(14-21)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
tsunami/engine/rootelem.go (1)
48-57: Thread-safety invariants — protect Root mutations or confine to a single goroutineNeedsRenderMap, EffectWorkQueue and RefOperations are mutated without a Root-level lock: AddRenderWork and AddEffectWork in tsunami/engine/rootelem.go write those fields without synchronisation, and AtomAddRenderWork holds r.atomLock then calls AddRenderWork (possible race between atomLock-protected code and other callers). Either add a Root-level mutex (or otherwise synchronise access to those fields) or ensure all mutations are serialized on a single event-loop goroutine — confirm the intended concurrency model.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (18)
tsunami/engine/globalctx.go (1)
22-31: Fix: single-slot globals lose context under concurrency; use per-goroutine stacks + RWMutexConcurrent renders/events/effects on different goroutines clobber each other, making GetGlobalXContext intermittently return nil. Use a map[gid][]ctx (stack) per kind to support re-entrancy and avoid overwrites. Also switch getters to RLock. This mirrors the earlier recommendation.
Apply:
@@ -var globalRenderContext *RenderContextImpl -var globalRenderGoId uint64 - -var globalEventContext *EventContextImpl -var globalEventGoId uint64 - -var globalEffectContext *EffectContextImpl -var globalEffectGoId uint64 - -var globalCtxMutex sync.Mutex +var globalCtxMutex sync.RWMutex +var globalRenderByGID = map[uint64][]*RenderContextImpl{} +var globalEventByGID = map[uint64][]*EventContextImpl{} +var globalEffectByGID = map[uint64][]*EffectContextImpl{} @@ -func setGlobalRenderContext(vc *RenderContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = vc - globalRenderGoId = goid.Get() -} +func setGlobalRenderContext(vc *RenderContextImpl) { + gid := goid.Get() + globalCtxMutex.Lock() + globalRenderByGID[gid] = append(globalRenderByGID[gid], vc) + globalCtxMutex.Unlock() +} @@ -func clearGlobalRenderContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalRenderContext = nil - globalRenderGoId = 0 -} +func clearGlobalRenderContext() { + gid := goid.Get() + globalCtxMutex.Lock() + stack := globalRenderByGID[gid] + if len(stack) > 0 { + stack = stack[:len(stack)-1] + } + if len(stack) == 0 { + delete(globalRenderByGID, gid) + } else { + globalRenderByGID[gid] = stack + } + globalCtxMutex.Unlock() +} @@ -func GetGlobalRenderContext() *RenderContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalRenderGoId { - return nil - } - return globalRenderContext -} +func GetGlobalRenderContext() *RenderContextImpl { + gid := goid.Get() + globalCtxMutex.RLock() + stack := globalRenderByGID[gid] + var vc *RenderContextImpl + if n := len(stack); n > 0 { + vc = stack[n-1] + } + globalCtxMutex.RUnlock() + return vc +} @@ -func setGlobalEventContext(ec *EventContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEventContext = ec - globalEventGoId = goid.Get() -} +func setGlobalEventContext(ec *EventContextImpl) { + gid := goid.Get() + globalCtxMutex.Lock() + globalEventByGID[gid] = append(globalEventByGID[gid], ec) + globalCtxMutex.Unlock() +} @@ -func clearGlobalEventContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEventContext = nil - globalEventGoId = 0 -} +func clearGlobalEventContext() { + gid := goid.Get() + globalCtxMutex.Lock() + stack := globalEventByGID[gid] + if len(stack) > 0 { + stack = stack[:len(stack)-1] + } + if len(stack) == 0 { + delete(globalEventByGID, gid) + } else { + globalEventByGID[gid] = stack + } + globalCtxMutex.Unlock() +} @@ -func GetGlobalEventContext() *EventContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalEventGoId { - return nil - } - return globalEventContext -} +func GetGlobalEventContext() *EventContextImpl { + gid := goid.Get() + globalCtxMutex.RLock() + stack := globalEventByGID[gid] + var ec *EventContextImpl + if n := len(stack); n > 0 { + ec = stack[n-1] + } + globalCtxMutex.RUnlock() + return ec +} @@ -func setGlobalEffectContext(ec *EffectContextImpl) { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEffectContext = ec - globalEffectGoId = goid.Get() -} +func setGlobalEffectContext(ec *EffectContextImpl) { + gid := goid.Get() + globalCtxMutex.Lock() + globalEffectByGID[gid] = append(globalEffectByGID[gid], ec) + globalCtxMutex.Unlock() +} @@ -func clearGlobalEffectContext() { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - globalEffectContext = nil - globalEffectGoId = 0 -} +func clearGlobalEffectContext() { + gid := goid.Get() + globalCtxMutex.Lock() + stack := globalEffectByGID[gid] + if len(stack) > 0 { + stack = stack[:len(stack)-1] + } + if len(stack) == 0 { + delete(globalEffectByGID, gid) + } else { + globalEffectByGID[gid] = stack + } + globalCtxMutex.Unlock() +} @@ -func GetGlobalEffectContext() *EffectContextImpl { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - gid := goid.Get() - if gid != globalEffectGoId { - return nil - } - return globalEffectContext -} +func GetGlobalEffectContext() *EffectContextImpl { + gid := goid.Get() + globalCtxMutex.RLock() + stack := globalEffectByGID[gid] + var ec *EffectContextImpl + if n := len(stack); n > 0 { + ec = stack[n-1] + } + globalCtxMutex.RUnlock() + return ec +} @@ -func inContextType() string { - globalCtxMutex.Lock() - defer globalCtxMutex.Unlock() - - gid := goid.Get() - - if globalRenderContext != nil && gid == globalRenderGoId { - return GlobalContextType_render - } - - if globalEventContext != nil && gid == globalEventGoId { - return GlobalContextType_event - } - - if globalEffectContext != nil && gid == globalEffectGoId { - return GlobalContextType_effect - } - - return GlobalContextType_async -} +func inContextType() string { + gid := goid.Get() + globalCtxMutex.RLock() + defer globalCtxMutex.RUnlock() + if s := globalRenderByGID[gid]; len(s) > 0 { + return GlobalContextType_render + } + if s := globalEventByGID[gid]; len(s) > 0 { + return GlobalContextType_event + } + if s := globalEffectByGID[gid]; len(s) > 0 { + return GlobalContextType_effect + } + return GlobalContextType_async +}If you prefer simpler semantics (no re-entrancy), replace the stacks with single entries per gid (map[uint64]*Ctx).
Also applies to: 44-56, 64-72, 74-86, 94-102, 104-116, 124-132, 141-159
tsunami/app/hooks.go (4)
21-29: Add nil-context guard in UseVDomRef for clearer failuresPanic early with a precise message instead of engine-level panic.
func UseVDomRef() *vdom.VDomRef { rc := engine.GetGlobalRenderContext() + if rc == nil { + panic("UseVDomRef must be called within a component (no context)") + } val := engine.UseVDomRef(rc) refVal, ok := val.(*vdom.VDomRef)
35-43: Add nil-context guard in UseRefMatch other hooks’ behavior; improves DX.
func UseRef[T any](val T) *vdom.VDomSimpleRef[T] { rc := engine.GetGlobalRenderContext() + if rc == nil { + panic("UseRef must be called within a component (no context)") + } refVal := engine.UseRef(rc, &vdom.VDomSimpleRef[T]{Current: val})
106-116: Fail fast if default client is nil in UseLocalAvoids later NPEs when operating on the Atom.
atomName := engine.UseLocal(rc, initialVal) - return Atom[T]{ - name: atomName, - client: engine.GetDefaultClient(), - } + client := engine.GetDefaultClient() + if client == nil { + panic("UseLocal: default client is not initialized") + } + return Atom[T]{name: atomName, client: client}
166-180: Validate ticker interval (> 0) to prevent time.NewTicker panictime.NewTicker panics for interval <= 0; guard upfront.
func UseTicker(interval time.Duration, tickFn func(), deps []any) { + if interval <= 0 { + panic("UseTicker: interval must be > 0") + } UseGoRoutine(func(ctx context.Context) { ticker := time.NewTicker(interval)tsunami/engine/hooks.go (2)
6-11: Import reflect for safe dependency comparisonsRequired by the depsEqual fix below.
import ( "log" "strconv" + "reflect" "github.com/wavetermdev/waveterm/tsunami/vdom" )
106-116: Fix: depsEqual can panic on uncomparable types; use safe, React-like semanticsDirect interface != panics for maps/slices/funcs. Compare safely: comparable types by value; for reference types use identity; fall back to DeepEqual for structs.
-func depsEqual(deps1 []any, deps2 []any) bool { - if len(deps1) != len(deps2) { - return false - } - for i := range deps1 { - if deps1[i] != deps2[i] { - return false - } - } - return true -} +func depsEqual(deps1 []any, deps2 []any) bool { + if len(deps1) != len(deps2) { + return false + } + for i := range deps1 { + d1, d2 := deps1[i], deps2[i] + if (d1 == nil) != (d2 == nil) { + return false + } + if d1 == nil { // both nil + continue + } + t1, t2 := reflect.TypeOf(d1), reflect.TypeOf(d2) + if t1 != t2 { + return false + } + if t1.Comparable() { + if d1 != d2 { + return false + } + continue + } + v1, v2 := reflect.ValueOf(d1), reflect.ValueOf(d2) + switch v1.Kind() { + case reflect.Slice, reflect.Map, reflect.Chan, reflect.Func, reflect.Pointer: + // React-like referential equality for reference types + if v1.IsNil() || v2.IsNil() { + if v1.IsNil() && v2.IsNil() { + continue + } + return false + } + if v1.Pointer() != v2.Pointer() { + return false + } + default: + // Non-comparable structs, arrays, etc. + if !reflect.DeepEqual(d1, d2) { + return false + } + } + } + return true +}tsunami/engine/clientimpl.go (5)
6-18: Fix uppercase check to handle multibyte UTF-8 component namesIndexing
name[0]breaks for multibyte runes. Decode the first rune with utf8 and import the package.import ( "context" "fmt" "io/fs" "log" "net" "net/http" "os" "strings" "sync" "time" "unicode" + "unicode/utf8" "github.com/google/uuid" "github.com/wavetermdev/waveterm/tsunami/rpctypes" "github.com/wavetermdev/waveterm/tsunami/util" "github.com/wavetermdev/waveterm/tsunami/vdom" )- if !unicode.IsUpper(rune(name[0])) { + r, _ := utf8.DecodeRuneInString(name) + if !unicode.IsUpper(r) { panic("Component name must start with an uppercase letter") }Also applies to: 241-247
111-113: Eliminate data race on GlobalEventHandler writeProtect the write with the client mutex; readers run concurrently.
func (c *ClientImpl) SetGlobalEventHandler(handler func(event vdom.VDomEvent)) { - c.GlobalEventHandler = handler + c.Lock.Lock() + c.GlobalEventHandler = handler + c.Lock.Unlock() }
127-133: Avoid concurrent read of GlobalEventHandler in makeBackendOptsCopy under lock to prevent races with setter.
func (c *ClientImpl) makeBackendOpts() *rpctypes.VDomBackendOpts { - return &rpctypes.VDomBackendOpts{ - Title: c.Root.AppTitle, - GlobalKeyboardEvents: c.GlobalEventHandler != nil, - FaviconPath: c.getFaviconPath(), - } + c.Lock.Lock() + hasGlobal := c.GlobalEventHandler != nil + c.Lock.Unlock() + return &rpctypes.VDomBackendOpts{ + Title: c.Root.AppTitle, + GlobalKeyboardEvents: hasGlobal, + FaviconPath: c.getFaviconPath(), + } }
115-125: Close files during favicon probing to avoid fd leaks
StaticFS.Openreturns an fs.File that must be closed.func (c *ClientImpl) getFaviconPath() string { if c.StaticFS != nil { faviconNames := []string{"favicon.ico", "favicon.png", "favicon.svg", "favicon.gif", "favicon.jpg"} for _, name := range faviconNames { - if _, err := c.StaticFS.Open(name); err == nil { - return "/static/" + name - } + if f, err := c.StaticFS.Open(name); err == nil { + _ = f.Close() + return "/static/" + name + } } } return "/wave-logo-256.png" }
200-207: Tie HTTP server lifetime to client DoneChWithout this, the server can outlive the client and hang the process.
// Wait for context cancellation and shutdown server gracefully 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) } }() + + // Also shut down when the client finishes + go func() { + <-c.DoneCh + if err := server.Shutdown(context.Background()); err != nil && err != http.ErrServerClosed { + log.Printf("Server shutdown on DoneCh error: %v", err) + } + }()tsunami/engine/rootelem.go (6)
213-216: Fix typo in user-facing error“cannot b nil” → “cannot be nil”.
- if cfunc == nil { - return fmt.Errorf("Component function cannot b nil") - } + if cfunc == nil { + return fmt.Errorf("Component function cannot be nil") + }
245-251: Reject duplicate component registrationOverwriting existing entries is error-prone.
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }
371-378: Guard effect hook index and nil to avoid panicsBounds-check EffectIndex and skip nil hooks in both cleanup and init passes.
for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - r.runEffectUnmount(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffectUnmount(work, hook) } // now run, new effects for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - r.runEffect(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffect(work, hook) }Also applies to: 380-387
389-392: Nil-guard root before renderingAvoid NPE if RunWork executes before first render.
- if len(renderIds) > 0 { - r.render(r.Root.Elem, &r.Root, "root", opts) - } + if len(renderIds) > 0 { + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } + }
35-48: Protect RefOperations with a mutexQueueing and draining happen across render/event paths; add a lock to avoid races.
type RootElem struct { Root *ComponentImpl RenderTs int64 AppTitle string CFuncs map[string]any // component name => render function CompMap map[string]*ComponentImpl // component waveid -> component EffectWorkQueue []*EffectWorkElem needsRenderMap map[string]bool // key: waveid needsRenderLock sync.Mutex Atoms map[string]genAtom // key: atomName atomLock sync.Mutex RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex Client *ClientImpl }func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }Also applies to: 428-436
184-193: Avoid lock coupling in SetAtomVal/SetFnAtomValDon’t hold atomLock while calling atom methods; reduces deadlock risk.
func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) } func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetFnVal(setFn) }Also applies to: 195-204
🧹 Nitpick comments (3)
tsunami/engine/globalctx.go (1)
38-42: Avoid stringly-typed WorkType; define typed constantsPrevent typos and enable autocomplete by defining constants (e.g., EffectWorkTypeRun/Unmount) instead of raw strings.
type EffectContextImpl struct { WorkElem EffectWorkElem - WorkType string // "run" or "unmount" + WorkType string // EffectWorkTypeRun or EffectWorkTypeUnmount Root *RootElem } + +const ( + EffectWorkTypeRun = "run" + EffectWorkTypeUnmount = "unmount" +)tsunami/app/hooks.go (1)
153-159: Minor: cancel is never nil; drop redundant checkSlight cleanup.
// Return cleanup function that cancels the context return func() { - if cancel != nil { - cancel() - } + cancel() }tsunami/engine/clientimpl.go (1)
189-192: Log the actual bound addressWhen TSUNAMI_LISTENADDR isn’t localhost (e.g., 0.0.0.0), the current log is misleading.
- port := listener.Addr().(*net.TCPAddr).Port - log.Printf("[tsunami] listening at http://localhost:%d", port) + log.Printf("[tsunami] listening at http://%s", listener.Addr().String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tsunami/app/hooks.go(1 hunks)tsunami/engine/clientimpl.go(1 hunks)tsunami/engine/globalctx.go(1 hunks)tsunami/engine/hooks.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tsunami/app/hooks.go (6)
tsunami/engine/hooks.go (8)
UseVDomRef(83-95)UseRef(97-104)UseId(64-66)UseRenderTs(60-62)UseResync(147-152)UseEffect(118-145)UseSetAppTitle(154-160)UseLocal(68-81)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)VDomSimpleRef(43-45)tsunami/engine/globalctx.go (1)
GetGlobalRenderContext(64-72)tsunami/app/atom.go (1)
Atom(62-65)tsunami/engine/clientimpl.go (1)
GetDefaultClient(71-73)tsunami/util/util.go (1)
PanicHandler(24-34)
tsunami/engine/hooks.go (6)
tsunami/engine/rootelem.go (1)
RootElem(35-48)tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/app/hooks.go (8)
UseRenderTs(59-65)UseId(49-55)UseLocal(106-116)UseVDomRef(21-29)UseRef(35-43)UseEffect(84-91)UseResync(71-77)UseSetAppTitle(95-101)tsunami/engine/atomimpl.go (1)
MakeAtomImpl(18-24)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)ObjectType_Ref(13-13)
tsunami/engine/clientimpl.go (5)
tsunami/engine/rootelem.go (2)
RootElem(35-48)MakeRoot(114-122)tsunami/vdom/vdom_types.go (3)
VDomElem(17-22)VDomEvent(65-75)WaveNullTag(8-8)tsunami/app/defaultclient.go (4)
SetGlobalEventHandler(19-21)RegisterSetupFn(26-28)SendAsyncInitiation(34-36)HandleDynFunc(65-67)tsunami/rpctypes/protocoltypes.go (4)
VDomBackendOpts(171-175)RenderedElem(13-19)VDomBackendUpdate(42-54)VDomRenderUpdate(177-183)tsunami/engine/render.go (1)
RenderOpts(19-21)
tsunami/engine/globalctx.go (3)
tsunami/engine/hooks.go (1)
RenderContextImpl(23-28)tsunami/vdom/vdom_types.go (1)
VDomEvent(65-75)tsunami/engine/rootelem.go (2)
RootElem(35-48)EffectWorkElem(21-25)
tsunami/engine/rootelem.go (9)
tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/engine/clientimpl.go (1)
ClientImpl(40-56)tsunami/engine/globalctx.go (3)
GlobalContextType_async(14-14)EventContextImpl(33-36)EffectContextImpl(38-42)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/hooks.go (1)
Hook(14-21)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
tsunami/engine/clientimpl.go (1)
159-169: Dyn handler wiring verified — UrlHandlerMux is bridged into the active mux.registerHandlers delegates to Client.UrlHandlerMux via h.Client.UrlHandlerMux.ServeHTTP(w, r) (serverhandlers.go:278), and HandleDynFunc registers dynamic routes on c.UrlHandlerMux (clientimpl.go:309). No change required.
| func (c *ClientImpl) RunEvents(events []vdom.VDomEvent) { | ||
| for _, event := range events { | ||
| c.Root.Event(event, c.GlobalEventHandler) | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard GlobalEventHandler read in RunEvents
Load the handler under lock to avoid races with SetGlobalEventHandler.
func (c *ClientImpl) RunEvents(events []vdom.VDomEvent) {
- for _, event := range events {
- c.Root.Event(event, c.GlobalEventHandler)
- }
+ c.Lock.Lock()
+ handler := c.GlobalEventHandler
+ c.Lock.Unlock()
+ for _, event := range events {
+ c.Root.Event(event, handler)
+ }
}🤖 Prompt for AI Agents
In tsunami/engine/clientimpl.go around lines 312 to 316, RunEvents currently
reads c.GlobalEventHandler without synchronization which can race with
SetGlobalEventHandler; acquire the appropriate mutex (e.g., c.mu or the existing
handler lock) and copy the GlobalEventHandler into a local variable while
holding the lock, then release the lock and iterate events calling the local
handler; also nil-check the copied handler before invoking to avoid panics.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
tsunami/demo/cpuchart/app.go (1)
40-49: Avoid doubled sampling interval: let the ticker drive cadence.cpu.Percent(time.Second, false) blocks ~1s; combined with UseTicker(time.Second) it yields ~2s updates. Use non-blocking read.
- percentages, err := cpu.Percent(time.Second, false) + percentages, err := cpu.Percent(0, false)tsunami/app/atom.go (1)
35-39: Honor docs: nil pointers must return false in sameRefTwo nil pointers currently compare equal; contradicts “Nil values return false.” Guard with IsZero first.
case reflect.Ptr: - // direct comparison works for *T - return any(oldVal) == any(newVal) + // guard nils to honor "nil values return false" + if vOld.IsZero() || vNew.IsZero() { + return false + } + // direct comparison works for *T + return any(oldVal) == any(newVal)tsunami/engine/clientimpl.go (6)
120-129: Close file when probing favicon to avoid fd leakClose the handle returned by StaticFS.Open before returning.
for _, name := range faviconNames { - if _, err := c.StaticFS.Open(name); err == nil { - return "/static/" + name - } + if f, err := c.StaticFS.Open(name); err == nil { + _ = f.Close() + return "/static/" + name + } }
116-118: Data race: SetGlobalEventHandler writes without syncProtect writes with the client mutex; mirror with locked reads (see below).
func (c *ClientImpl) SetGlobalEventHandler(handler func(event vdom.VDomEvent)) { - c.GlobalEventHandler = handler + c.Lock.Lock() + c.GlobalEventHandler = handler + c.Lock.Unlock() }
132-138: Data race: makeBackendOpts reads GlobalEventHandler without syncCopy the bool under lock.
func (c *ClientImpl) makeBackendOpts() *rpctypes.VDomBackendOpts { - return &rpctypes.VDomBackendOpts{ - Title: c.Root.AppTitle, - GlobalKeyboardEvents: c.GlobalEventHandler != nil, - FaviconPath: c.getFaviconPath(), - } + c.Lock.Lock() + hasGlobal := c.GlobalEventHandler != nil + c.Lock.Unlock() + return &rpctypes.VDomBackendOpts{ + Title: c.Root.AppTitle, + GlobalKeyboardEvents: hasGlobal, + FaviconPath: c.getFaviconPath(), + } }
314-318: Data race: RunEvents reads GlobalEventHandler without syncLoad handler under lock once, then iterate.
func (c *ClientImpl) RunEvents(events []vdom.VDomEvent) { - for _, event := range events { - c.Root.Event(event, c.GlobalEventHandler) - } + c.Lock.Lock() + handler := c.GlobalEventHandler + c.Lock.Unlock() + for _, event := range events { + c.Root.Event(event, handler) + } }
205-214: Server lifetime not tied to client DoneChAdd shutdown on DoneCh to avoid leaking the server after client completes.
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) } }() + + // Also shut down when the client finishes + go func() { + <-c.DoneCh + if err := server.Shutdown(context.Background()); err != nil && err != http.ErrServerClosed { + log.Printf("Server shutdown on DoneCh error: %v", err) + } + }()
6-24: Unicode: use first rune, not first byte for uppercase checkmulti-byte names break with rune(name[0]). Decode first rune.
import ( ... - "unicode" + "unicode" + "unicode/utf8" )- if !unicode.IsUpper(rune(name[0])) { + r, _ := utf8.DecodeRuneInString(name) + if !unicode.IsUpper(r) { panic("Component name must start with an uppercase letter") }Also applies to: 243-249
tsunami/engine/rootelem.go (6)
201-204: Typo in error message“cannot b nil” → “cannot be nil”.
- return fmt.Errorf("Component function cannot b nil") + return fmt.Errorf("Component function cannot be nil")
233-239: Reject duplicate component registrationsSilently overwriting breaks invariants. Return an error if already present.
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }
359-366: Guard effect index and nil hooks in RunWorkIndexing comp.Hooks without bounds/nil checks can panic.
- hook := comp.Hooks[work.EffectIndex] - r.runEffectUnmount(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffectUnmount(work, hook)- hook := comp.Hooks[work.EffectIndex] - r.runEffect(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffect(work, hook)Also applies to: 368-375
377-380: Nil-guard root before calling renderRunWork can execute before Root/Root.Elem is set.
- if len(renderIds) > 0 { - r.render(r.Root.Elem, &r.Root, "root", opts) - } + if len(renderIds) > 0 { + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } + }
183-192: Avoid lock coupling on SetAtomValHolding atomLock while calling atom.SetVal risks deadlocks if atom has its own lock.
func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) }
34-47: Protect RefOperations with a mutexQueueRefOp/GetRefOperations mutate shared slice; add a lock.
type RootElem struct { ... - RefOperations []vdom.VDomRefOperation + RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex Client *ClientImpl }func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }Also applies to: 416-424
🧹 Nitpick comments (4)
tsunami/demo/cpuchart/app.go (2)
51-67: Treat collection failures as missing data and stop per-tick info logging.Returning a 0% point on error skews stats; use nil to mark missing data and only log errors.
func generateCPUDataPoint() CPUDataPoint { now := time.Now() cpuUsage, err := collectCPUUsage() - if err != nil { - log.Printf("Error collecting CPU usage: %v", err) - cpuUsage = 0 - } + var cpuPtr *float64 + if err != nil { + log.Printf("Error collecting CPU usage: %v", err) + } else { + cpuPtr = &cpuUsage + } dataPoint := CPUDataPoint{ Time: now.Unix(), - CPUUsage: &cpuUsage, // Convert to pointer + CPUUsage: cpuPtr, Timestamp: now.Format("15:04:05"), } - log.Printf("CPU Usage: %.2f%% at %s", cpuUsage, dataPoint.Timestamp) return dataPoint }
209-211: Make “60 seconds” dynamic to match the configured window.Reflect dataPointCountAtom instead of hardcoding 60.
- }, "Live CPU usage data collected using gopsutil, displaying 60 seconds of history"), + }, "Live CPU usage data collected using gopsutil, displaying ", dataPointCount, " seconds of history"),- "Rolling window of 60 seconds of historical data", + "Rolling window of ", dataPointCount, " seconds of historical data",Also applies to: 329-337
tsunami/engine/asyncnotify.go (1)
15-21: Remove noisy hot-path log.This runs on every notify; it will spam logs and add overhead.
func (c *ClientImpl) notifyAsyncRenderWork() { - log.Printf("notify async work\n") c.notifyOnce.Do(func() { c.notifyWakeCh = make(chan struct{}, 1) go c.asyncInitiationLoop() })tsunami/engine/atomimpl.go (1)
12-16: Optional: RWMutex for higher read concurrency.GetVal and GetUsedBy are read-heavy; switching to sync.RWMutex can reduce contention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tsunami/app/atom.go(1 hunks)tsunami/demo/cpuchart/app.go(1 hunks)tsunami/demo/cpuchart/static/tw.css(1 hunks)tsunami/engine/asyncnotify.go(1 hunks)tsunami/engine/atomimpl.go(1 hunks)tsunami/engine/clientimpl.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsunami/demo/cpuchart/static/tw.css
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-23T06:09:05.334Z
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1807
File: pkg/wshrpc/wshremote/wshremote.go:718-726
Timestamp: 2025-01-23T06:09:05.334Z
Learning: In Go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.Field != nil`) is the idiomatic way to handle multiple nil checks. Avoid suggesting nested if statements as they make the code more verbose without adding value.
Applied to files:
tsunami/app/atom.go
🧬 Code graph analysis (5)
tsunami/engine/asyncnotify.go (2)
tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/app/defaultclient.go (1)
SendAsyncInitiation(34-36)
tsunami/demo/cpuchart/app.go (3)
tsunami/app/defaultclient.go (3)
ConfigAtom(38-44)DataAtom(46-52)DefineComponent(15-17)tsunami/vdom/vdom.go (1)
H(91-100)tsunami/app/hooks.go (2)
UseSetAppTitle(95-101)UseTicker(166-180)
tsunami/app/atom.go (4)
tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/engine/globalctx.go (1)
GetGlobalRenderContext(64-72)tsunami/util/util.go (1)
GetTypedAtomValue(103-121)tsunami/app/defaultclient.go (1)
DeepCopy(84-95)
tsunami/engine/rootelem.go (9)
tsunami/engine/comp.go (1)
ComponentImpl(21-42)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/engine/globalctx.go (3)
GlobalContextType_async(14-14)EventContextImpl(33-36)EffectContextImpl(38-42)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/hooks.go (1)
Hook(14-21)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
tsunami/engine/clientimpl.go (5)
tsunami/engine/rootelem.go (2)
RootElem(34-47)MakeRoot(113-121)tsunami/vdom/vdom_types.go (3)
VDomElem(17-22)VDomEvent(65-75)WaveNullTag(8-8)tsunami/app/defaultclient.go (5)
SetGlobalEventHandler(19-21)RegisterSetupFn(26-28)RunMain(70-72)SendAsyncInitiation(34-36)HandleDynFunc(65-67)tsunami/rpctypes/protocoltypes.go (4)
VDomBackendOpts(171-175)RenderedElem(13-19)VDomBackendUpdate(42-54)VDomRenderUpdate(177-183)tsunami/engine/render.go (1)
RenderOpts(19-21)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
| // SetFn updates the atom's value by applying the provided function to the current value. | ||
| // The function receives a copy of the current atom value, which can be safely mutated | ||
| // without affecting the original data. The return value from the function becomes the | ||
| // new atom value. This method cannot be called during render cycles. | ||
| func (a Atom[T]) SetFn(fn func(T) T) { | ||
| vc := engine.GetGlobalRenderContext() | ||
| if vc != nil { | ||
| logInvalidAtomSet(a.name) | ||
| return | ||
| } | ||
| currentVal := a.Get() | ||
| copiedVal := DeepCopy(currentVal) | ||
| newVal := fn(copiedVal) | ||
| a.Set(newVal) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SetFn is not atomic; risk of lost updates
Get–transform–Set can lose concurrent updates. Do the read-modify-write atomically in engine (per-atom lock), and keep the DeepCopy + mutation-warning logic.
Apply in this file:
func (a Atom[T]) SetFn(fn func(T) T) {
vc := engine.GetGlobalRenderContext()
if vc != nil {
logInvalidAtomSet(a.name)
return
}
- currentVal := a.Get()
- copiedVal := DeepCopy(currentVal)
- newVal := fn(copiedVal)
- a.Set(newVal)
+ err := a.client.Root.SetFnAtomVal(a.name, func(val any) any {
+ typedVal := util.GetTypedAtomValue[T](val, a.name)
+ copied := DeepCopy(typedVal)
+ result := fn(copied)
+ if sameRef(typedVal, result) {
+ logMutationWarning(a.name)
+ }
+ return result
+ })
+ if err != nil {
+ log.Printf("Failed to SetFn atom %s: %v", a.name, err)
+ return
+ }
+ a.client.Root.AtomAddRenderWork(a.name)
}And in engine/rootelem.go (see comments there) reintroduce SetFnAtomVal and extend genAtom with SetFnVal(fn func(any) any) error, performing lookup under r.atomLock then calling atom.SetFnVal outside that lock. Please verify genAtom implementations already support SetFnVal.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root ==="
ls -la
echo "=== ripgrep for relevant symbols ==="
rg -n --hidden "SetFnAtomVal|SetFnVal|AtomAddRenderWork|AtomAddRender|genAtom|rootelem|atomLock|DeepCopy|GetTypedAtomValue|sameRef" -S || true
echo "=== show tsunami/app/atom.go (first 240 lines) ==="
if [ -f "tsunami/app/atom.go" ]; then
sed -n '1,240p' tsunami/app/atom.go
else
echo "tsunami/app/atom.go not found"
fi
echo "=== show engine/rootelem.go (first 400 lines) ==="
if [ -f "engine/rootelem.go" ]; then
sed -n '1,400p' engine/rootelem.go
else
echo "engine/rootelem.go not found"
fi
echo "=== list engine/ dir ==="
ls -la engine || true
echo "=== search for files named like gen*atom*.go or gen_*atom*.go ==="
rg -n --hidden "gen.*atom" -S || true
echo "=== search for methods named SetFnVal or SetFnVal signatures ==="
rg -n --hidden "func\s+\([^\)]*\)\s+SetFnVal\(" -S || true
echo "=== done ==="Length of output: 12395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show tsunami/engine/rootelem.go (first 400 lines) ==="
if [ -f "tsunami/engine/rootelem.go" ]; then
sed -n '1,400p' tsunami/engine/rootelem.go
else
echo "tsunami/engine/rootelem.go not found"
fi
echo
echo "=== search for genAtom and related methods across tsunami/ ==="
rg -n --hidden "type genAtom interface|SetAtomVal\(|GetAtomVal\(|SetFnVal\(|SetFnAtomVal\(|AtomAddRenderWork\(|AtomSetUsedBy\(" tsunami -S || true
echo
echo "=== search for implementations that might satisfy genAtom ==="
rg -n --hidden "struct .*Atom|func .*SetAtomVal\(|func .*SetFnVal\(" tsunami -S || true
echo
echo "=== show tsunami/util/util.go (GetTypedAtomValue) ==="
sed -n '1,240p' tsunami/util/util.go || true
echo
echo "=== done ==="Length of output: 16884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== list tsunami/engine ==="
ls -la tsunami/engine || true
echo
echo "=== search for GetVal/SetVal/SetUsedBy/GetUsedBy implementations ==="
rg -n --hidden -S "func\s+\([^)]+\)\s+GetVal\(" tsunami || true
rg -n --hidden -S "func\s+\([^)]+\)\s+SetVal\(" tsunami || true
rg -n --hidden -S "func\s+\([^)]+\)\s+SetUsedBy\(" tsunami || true
rg -n --hidden -S "func\s+\([^)]+\)\s+GetUsedBy\(" tsunami || true
echo
echo "=== search for Atom struct types ==="
rg -n --hidden -S "type\s+\w*Atom\s+struct" tsunami || true
echo
echo "=== show files that implement genAtom by looking for SetVal/GetVal usages ==="
rg -n --hidden -S "SetVal\\(|GetVal\\(" tsunami -g '!**/node_modules/**' -C3 || true
echo
echo "=== done ==="Length of output: 1283
Make SetFn atomic — move read→modify→write into the engine
- Problem: Atom.SetFn currently does Get → DeepCopy → fn → Set outside engine locks; this can lose concurrent updates.
- Fix (high level): Replace Atom.SetFn to call a.client.Root.SetFnAtomVal(a.name, func(val any) any { typed := util.GetTypedAtomValue[T](val, a.name); copied := DeepCopy(typed); result := fn(copied); if sameRef(typed, result) { logMutationWarning(a.name) }; return result }) — handle error and then call AtomAddRenderWork.
- Engine changes required: add RootElem.SetFnAtomVal(name string, fn func(any) any) error which looks up the atom under r.atomLock and then invokes atom.SetFnVal(fn) outside the lock; extend genAtom with SetFnVal(func(any) any) error and implement SetFnVal in tsunami/engine/atomimpl.go (do the DeepCopy + mutation-warning logic there).
- Verified: genAtom currently has only GetVal, SetVal, SetUsedBy, GetUsedBy — no SetFnVal/SetFnAtomVal exist, so the engine changes above are mandatory.
- Files to modify: tsunami/app/atom.go, tsunami/engine/rootelem.go, tsunami/engine/atomimpl.go.
🤖 Prompt for AI Agents
In tsunami/app/atom.go around lines 110 to 124, SetFn performs
Get→DeepCopy→fn→Set outside engine locks which can lose concurrent updates;
change it to call the engine API instead: invoke
a.client.Root.SetFnAtomVal(a.name, func(val any) any { typed :=
util.GetTypedAtomValue[T](val, a.name); copied := DeepCopy(typed); result :=
fn(copied); if sameRef(typed, result) { logMutationWarning(a.name) }; return
result }) and handle returned error (log/return) and then call AtomAddRenderWork
as before. To support this add RootElem.SetFnAtomVal(name string, fn func(any)
any) error in tsunami/engine/rootelem.go which finds the atom under r.atomLock
and then calls atom.SetFnVal(fn) outside the lock; extend generated atom
interface (genAtom) with SetFnVal(func(any) any) error and implement SetFnVal in
tsunami/engine/atomimpl.go to perform the DeepCopy + mutation-warning logic and
set the new value safely. Ensure errors are propagated and atom render work is
scheduled after a successful update.
| for { | ||
| select { | ||
| case <-c.notifyWakeCh: | ||
| schedule() | ||
|
|
||
| case <-timerC: | ||
| now := time.Now() | ||
|
|
||
| // Recompute right before sending; if a late event arrived, | ||
| // push the fire time out to respect the debounce. | ||
| firstNs := c.notifyBatchStartNs.Load() | ||
| if firstNs == 0 { | ||
| // Nothing to do. | ||
| continue | ||
| } | ||
| lastNs := c.notifyLastEventNs.Load() | ||
|
|
||
| first := time.Unix(0, firstNs) | ||
| last := time.Unix(0, lastNs) | ||
| cadenceReady := lastSent.Add(NotifyMaxCadence) | ||
|
|
||
| anchor := first | ||
| if cadenceReady.After(anchor) { | ||
| anchor = cadenceReady | ||
| } | ||
| deadline := anchor.Add(NotifyMaxDebounceTime) | ||
|
|
||
| candidate := last.Add(NotifyDebounceTime) | ||
| if deadline.Before(candidate) { | ||
| candidate = deadline | ||
| } | ||
| target := candidate | ||
| if cadenceReady.After(target) { | ||
| target = cadenceReady | ||
| } | ||
|
|
||
| // If we're early (because a new event just came in), reschedule. | ||
| if now.Before(target) { | ||
| d := time.Until(target) | ||
| if d < 0 { | ||
| d = 0 | ||
| } | ||
| if !timer.Stop() { | ||
| select { | ||
| case <-timer.C: | ||
| default: | ||
| } | ||
| } | ||
| timer.Reset(d) | ||
| continue | ||
| } | ||
|
|
||
| // Fire. | ||
| _ = c.SendAsyncInitiation() | ||
| lastSent = now | ||
|
|
||
| // Close current batch; a concurrent notify will CAS a new start. | ||
| c.notifyBatchStartNs.Store(0) | ||
|
|
||
| // If anything is already pending, this will arm the next timer. | ||
| schedule() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shutdown path to prevent goroutine/timer leaks.
asyncInitiationLoop never exits; tie it to ClientImpl.DoneCh and clean up timer.
func (c *ClientImpl) asyncInitiationLoop() {
var (
lastSent time.Time
timer *time.Timer
timerC <-chan time.Time
)
@@
for {
select {
+ case <-c.DoneCh:
+ if timer != nil {
+ if !timer.Stop() {
+ select {
+ case <-timer.C:
+ default:
+ }
+ }
+ }
+ return
case <-c.notifyWakeCh:
schedule()
case <-timerC:
now := time.Now()📝 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.
| for { | |
| select { | |
| case <-c.notifyWakeCh: | |
| schedule() | |
| case <-timerC: | |
| now := time.Now() | |
| // Recompute right before sending; if a late event arrived, | |
| // push the fire time out to respect the debounce. | |
| firstNs := c.notifyBatchStartNs.Load() | |
| if firstNs == 0 { | |
| // Nothing to do. | |
| continue | |
| } | |
| lastNs := c.notifyLastEventNs.Load() | |
| first := time.Unix(0, firstNs) | |
| last := time.Unix(0, lastNs) | |
| cadenceReady := lastSent.Add(NotifyMaxCadence) | |
| anchor := first | |
| if cadenceReady.After(anchor) { | |
| anchor = cadenceReady | |
| } | |
| deadline := anchor.Add(NotifyMaxDebounceTime) | |
| candidate := last.Add(NotifyDebounceTime) | |
| if deadline.Before(candidate) { | |
| candidate = deadline | |
| } | |
| target := candidate | |
| if cadenceReady.After(target) { | |
| target = cadenceReady | |
| } | |
| // If we're early (because a new event just came in), reschedule. | |
| if now.Before(target) { | |
| d := time.Until(target) | |
| if d < 0 { | |
| d = 0 | |
| } | |
| if !timer.Stop() { | |
| select { | |
| case <-timer.C: | |
| default: | |
| } | |
| } | |
| timer.Reset(d) | |
| continue | |
| } | |
| // Fire. | |
| _ = c.SendAsyncInitiation() | |
| lastSent = now | |
| // Close current batch; a concurrent notify will CAS a new start. | |
| c.notifyBatchStartNs.Store(0) | |
| // If anything is already pending, this will arm the next timer. | |
| schedule() | |
| } | |
| } | |
| } | |
| for { | |
| select { | |
| case <-c.DoneCh: | |
| if timer != nil { | |
| if !timer.Stop() { | |
| select { | |
| case <-timer.C: | |
| default: | |
| } | |
| } | |
| } | |
| return | |
| case <-c.notifyWakeCh: | |
| schedule() | |
| case <-timerC: | |
| now := time.Now() | |
| // Recompute right before sending; if a late event arrived, | |
| // push the fire time out to respect the debounce. | |
| firstNs := c.notifyBatchStartNs.Load() | |
| if firstNs == 0 { | |
| // Nothing to do. | |
| continue | |
| } | |
| lastNs := c.notifyLastEventNs.Load() | |
| first := time.Unix(0, firstNs) | |
| last := time.Unix(0, lastNs) | |
| cadenceReady := lastSent.Add(NotifyMaxCadence) | |
| anchor := first | |
| if cadenceReady.After(anchor) { | |
| anchor = cadenceReady | |
| } | |
| deadline := anchor.Add(NotifyMaxDebounceTime) | |
| candidate := last.Add(NotifyDebounceTime) | |
| if deadline.Before(candidate) { | |
| candidate = deadline | |
| } | |
| target := candidate | |
| if cadenceReady.After(target) { | |
| target = cadenceReady | |
| } | |
| // If we're early (because a new event just came in), reschedule. | |
| if now.Before(target) { | |
| d := time.Until(target) | |
| if d < 0 { | |
| d = 0 | |
| } | |
| if !timer.Stop() { | |
| select { | |
| case <-timer.C: | |
| default: | |
| } | |
| } | |
| timer.Reset(d) | |
| continue | |
| } | |
| // Fire. | |
| _ = c.SendAsyncInitiation() | |
| lastSent = now | |
| // Close current batch; a concurrent notify will CAS a new start. | |
| c.notifyBatchStartNs.Store(0) | |
| // If anything is already pending, this will arm the next timer. | |
| schedule() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In tsunami/engine/asyncnotify.go around lines 101 to 164, the
asyncInitiationLoop currently never exits causing goroutine and timer leaks; add
a shutdown path that listens for ClientImpl.DoneCh (or the client's context done
channel) in the select loop, and on receiving it stop the timer, drain timer.C
if needed, and return to exit the goroutine; ensure timer.Stop() is called
before returning and any resources (e.g., clearing notifyBatchStartNs or other
state) are cleaned up as appropriate so no timers or blocked sends remain.
| func (a *AtomImpl[T]) SetVal(val any) error { | ||
| a.lock.Lock() | ||
| defer a.lock.Unlock() | ||
| return a.setVal_nolock(val) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid holding the mutex during JSON (hot path contention).
Compute the adapted value first, then lock only for assignment.
-func (a *AtomImpl[T]) setVal_nolock(val any) error {
- if val == nil {
- var zero T
- a.val = zero
- return nil
- }
-
- // Try direct assignment if it's already type T
- if typed, ok := val.(T); ok {
- a.val = typed
- return nil
- }
-
- // Try JSON marshaling/unmarshaling
- jsonBytes, err := json.Marshal(val)
- if err != nil {
- var result T
- return fmt.Errorf("failed to adapt type from %T => %T, input type failed to marshal: %w", val, result, err)
- }
-
- var result T
- if err := json.Unmarshal(jsonBytes, &result); err != nil {
- return fmt.Errorf("failed to adapt type from %T => %T: %w", val, result, err)
- }
-
- a.val = result
- return nil
-}
+func (a *AtomImpl[T]) adaptValue(val any) (T, error) {
+ var out T
+ if val == nil {
+ return out, nil
+ }
+ if typed, ok := val.(T); ok {
+ return typed, nil
+ }
+ jsonBytes, err := json.Marshal(val)
+ if err != nil {
+ var target T
+ return out, fmt.Errorf("failed to adapt type from %T => %T, input type failed to marshal: %w", val, target, err)
+ }
+ if err := json.Unmarshal(jsonBytes, &out); err != nil {
+ var target T
+ return out, fmt.Errorf("failed to adapt type from %T => %T: %w", val, target, err)
+ }
+ return out, nil
+}
@@
func (a *AtomImpl[T]) SetVal(val any) error {
- a.lock.Lock()
- defer a.lock.Unlock()
- return a.setVal_nolock(val)
+ newVal, err := a.adaptValue(val)
+ if err != nil {
+ return err
+ }
+ a.lock.Lock()
+ a.val = newVal
+ a.lock.Unlock()
+ return nil
}Also applies to: 32-59
| type genAtom interface { | ||
| GetVal() any | ||
| SetVal(any) error | ||
| SetUsedBy(string, bool) | ||
| GetUsedBy() []string | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reintroduce atomic SetFn for atoms
Expose SetFn at root and in genAtom to support atomic read-modify-write (used by app.Atom.SetFn).
type genAtom interface {
GetVal() any
SetVal(any) error
+ SetFnVal(func(any) any) error
SetUsedBy(string, bool)
GetUsedBy() []string
}+func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error {
+ r.atomLock.Lock()
+ atom, ok := r.Atoms[name]
+ r.atomLock.Unlock()
+ if !ok {
+ return fmt.Errorf("atom %q not found", name)
+ }
+ return atom.SetFnVal(setFn)
+}Please confirm genAtom implementations define SetFnVal accordingly.
Also applies to: 193-199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "CWD: %s\n\n" "$(pwd)"
printf "1) Search for 'type genAtom' occurrences:\n"
rg -n --hidden --no-ignore-vcs "type genAtom" -S || true
printf "\n2) Show tsunami/engine/rootelem.go (first 240 lines) if present:\n"
if [ -f tsunami/engine/rootelem.go ]; then
sed -n '1,240p' tsunami/engine/rootelem.go || true
else
echo "tsunami/engine/rootelem.go not found"
fi
printf "\n3) Search for 'SetFnVal' in repo:\n"
rg -n --hidden --no-ignore-vcs "SetFnVal" -S || true
printf "\n4) Search for 'SetFn' occurrences (defs and calls):\n"
rg -n --hidden --no-ignore-vcs "\bSetFn\b|\bSetFnVal\b|\bSetFnAtomVal\b" -S || true
printf "\n5) Search for Go methods 'SetVal(' and 'GetVal(' with context:\n"
rg -n --hidden --no-ignore-vcs --type go "func\s+\([^\)]*\)\s+SetVal\(" -S -C3 || true
rg -n --hidden --no-ignore-vcs --type go "func\s+\([^\)]*\)\s+GetVal\(" -S -C3 || true
printf "\n6) List tsunami/engine directory if it exists:\n"
if [ -d tsunami/engine ]; then
ls -la tsunami/engine || true
else
echo "tsunami/engine directory not found"
fiLength of output: 10483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "CWD: %s\n\n" "$(pwd)"
printf "---- tsunami/engine/atomimpl.go ----\n\n"
if [ -f tsunami/engine/atomimpl.go ]; then
sed -n '1,240p' tsunami/engine/atomimpl.go || true
else
echo "tsunami/engine/atomimpl.go not found"
fi
printf "\n---- tsunami/app/atom.go ----\n\n"
if [ -f tsunami/app/atom.go ]; then
sed -n '1,240p' tsunami/app/atom.go || true
else
echo "tsunami/app/atom.go not found"
fi
printf "\n---- grep for SetFn in engine package ----\n\n"
rg -n --hidden --no-ignore-vcs --type go --glob '!**/vendor/**' "\bSetFn\b|\bSetFnVal\b" tsunami/engine || true
printf "\n---- show tsunami/engine/rootelem.go around Get/Set Atom methods ----\n\n"
if [ -f tsunami/engine/rootelem.go ]; then
sed -n '1,260p' tsunami/engine/rootelem.go || true
else
echo "tsunami/engine/rootelem.go not found"
fiLength of output: 120
Reintroduce atomic SetFn for atoms
Expose SetFn at Root and in genAtom to support atomic read-modify-write (used by app.Atom.SetFn). Add an interface method and a root accessor:
type genAtom interface {
GetVal() any
SetVal(any) error
+ SetFnVal(func(any) any) error
SetUsedBy(string, bool)
GetUsedBy() []string
}+func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error {
+ r.atomLock.Lock()
+ atom, ok := r.Atoms[name]
+ r.atomLock.Unlock()
+ if !ok {
+ return fmt.Errorf("atom %q not found", name)
+ }
+ return atom.SetFnVal(setFn)
+}Implement SetFnVal on AtomImpl (tsunami/engine/atomimpl.go) to perform atomic read-modify-write under the atom's lock (deep-copy current value as needed). Confirm all genAtom implementations define SetFnVal.
📝 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.
| type genAtom interface { | |
| GetVal() any | |
| SetVal(any) error | |
| SetUsedBy(string, bool) | |
| GetUsedBy() []string | |
| } | |
| type genAtom interface { | |
| GetVal() any | |
| SetVal(any) error | |
| SetFnVal(func(any) any) error | |
| SetUsedBy(string, bool) | |
| GetUsedBy() []string | |
| } | |
| func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error { | |
| r.atomLock.Lock() | |
| atom, ok := r.Atoms[name] | |
| r.atomLock.Unlock() | |
| if !ok { | |
| return fmt.Errorf("atom %q not found", name) | |
| } | |
| return atom.SetFnVal(setFn) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
tsunami/prompts/system.md (6)
155-161: Avoid index keys in list example; use stable keys.Index keys break reconciliation on reorders. Prefer a stable id/value.
- "key": idx, + "key": item, // or a stable id
219-225: Repeat: avoid index keys here too.- "key": idx, + "key": item, // or a stable id
255-256: Fix UseEffect signature in example.app.UseEffect expects fn func() func(). Return a cleanup (or nil).
- app.UseEffect(func() { /* effect */ }, nil) + app.UseEffect(func() func() { + /* effect */ + return nil + }, nil)
656-657: Use href (not src) on link tag.HTML link elements load styles via href.
-vdom.H("link", map[string]any{"rel": "stylesheet", "src": "/static/mystyles.css"}) +vdom.H("link", map[string]any{"rel": "stylesheet", "href": "/static/mystyles.css"})
1177-1183: Remove stray_ "embed"import (or demonstrate go:embed properly).As-is it breaks copy/paste. No embed usage in the template.
import ( - _ "embed" - "github.com/wavetermdev/waveterm/tsunami/app" "github.com/wavetermdev/waveterm/tsunami/vdom" )
1231-1247: Template mutates backing slice; use SetFn + DeepCopy and increment id safely.Align with your “no mutation” guidance and avoid Get/Set races.
- addTodo := func() { - currentInput := inputText.Get() - if currentInput == "" { - return - } - currentTodos := todos.Get() - currentNextId := nextId.Get() - - todos.Set(append(currentTodos, Todo{ - Id: currentNextId, - Text: currentInput, - Completed: false, - })) - nextId.Set(currentNextId + 1) - inputText.Set("") - } + addTodo := func() { + currentInput := inputText.Get() + if currentInput == "" { + return + } + next := nextId.Get() + todos.SetFn(func(cur []Todo) []Todo { + copySlice := app.DeepCopy(cur) + return append(copySlice, Todo{Id: next, Text: currentInput, Completed: false}) + }) + nextId.Set(next + 1) + inputText.Set("") + }tsunami/engine/asyncnotify.go (1)
33-39: Add shutdown path tied to DoneCh; stop/drain timer to avoid goroutine/timer leaks.The loop never exits and the timer can leak. Listen on
c.DoneChand ensure the timer is stopped and drained on exit.func (c *ClientImpl) asyncInitiationLoop() { var ( lastSent time.Time timer *time.Timer timerC <-chan time.Time ) + // Ensure the timer is cleaned up when the goroutine exits. + defer func() { + if timer != nil { + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + } + }() @@ for { select { + case <-c.DoneCh: + return case <-c.notifyWakeCh: schedule() case <-timerC:Also applies to: 99-161
tsunami/demo/pomodoro/app.go (1)
112-139: Nice: migrated to UseTicker — prior ticker/done races are gone.Using app.UseTicker with context cancellation replaces the ad-hoc ticker goroutine and removes the nil-channel/double-close risks flagged earlier. Good call.
🧹 Nitpick comments (10)
tsunami/prompts/system.md (3)
1091-1099: Add language to fenced block (markdownlint MD040).Use text for directory tree blocks.
-``` +```text your-app/ ... -``` +```
1353-1355: Use a heading instead of emphasis (markdownlint MD036).-**Async Operation Guidelines** +### Async Operation Guidelines
57-67: Replace hard tabs with spaces (markdownlint MD010).Consistently use spaces in this markdown file (including code blocks) to satisfy the linter.
If you want, I can run a quick sed-based pass to convert tabs to 4 spaces across this file.
Also applies to: 253-262, 541-555, 1178-1182
tsunami/engine/asyncnotify.go (4)
10-12: Exported constants need GoDoc (or make them unexported).Public identifiers without comments will fail linting and reduce discoverability.
-const NotifyMaxCadence = 10 * time.Millisecond -const NotifyDebounceTime = 500 * time.Microsecond -const NotifyMaxDebounceTime = 2 * time.Millisecond +// NotifyMaxCadence limits how frequently async initiations are sent. +const NotifyMaxCadence = 10 * time.Millisecond +// NotifyDebounceTime delays firing to coalesce closely spaced events. +const NotifyDebounceTime = 500 * time.Microsecond +// NotifyMaxDebounceTime caps the coalescing window per batch. +const NotifyMaxDebounceTime = 2 * time.Millisecond
55-59: Mixing time.Now with time.Unix drops monotonic clock; consider storing time.Time.Reconstructing
time.Timeviatime.Unixstrips the monotonic component, making debounce/cadence sensitive to wall-clock changes. Prefer storingtime.Time(fromtime.Now()) atomically (e.g., atomic.Value or atomic.Pointer) and operating purely on monotonic times.If helpful, I can draft a small refactor plan to switch
notifyBatchStartNs/notifyLastEventNsto monotonic-preserving types and adapt the scheduling math.Also applies to: 116-134
151-154: Surface SendAsyncInitiation errors (log/metric); don’t silently drop.Silent drops make diagnosing lost initiations hard.
- // Fire. - _ = c.SendAsyncInitiation() + // Fire. + if err := c.SendAsyncInitiation(); err != nil { + // TODO: log or increment a metric; swallow to avoid disrupting the loop + } lastSent = now
40-97: DRY the target-time computation to a helper to reduce duplication.The cadence/debounce math is duplicated in
schedule()and the timer branch; extract to a small helper for maintainability.Also applies to: 104-134
tsunami/demo/pomodoro/app.go (3)
130-137: Avoid float rounding jitter in m/s computation.Use integer duration math to prevent occasional off-by-one due to float truncation.
- m := int(remaining.Minutes()) - s := int(remaining.Seconds()) % 60 + m := int(remaining / time.Minute) + s := int((remaining % time.Minute) / time.Second)
27-33: Prefer typed Mode over raw int for OnMode.Passing Mode avoids brittle equality on minutes and eases future extension (e.g., custom durations).
type ControlButtonsProps struct { IsRunning bool `json:"isRunning"` OnStart func() `json:"onStart"` OnPause func() `json:"onPause"` OnReset func() `json:"onReset"` - OnMode func(int) `json:"onMode"` + OnMode func(Mode) `json:"onMode"` } @@ - vdom.H("button", + vdom.H("button", map[string]any{ "className": "flex-1 px-3 py-3 text-base border-none rounded bg-green-500 text-white cursor-pointer hover:bg-green-600 transition-colors duration-200", - "onClick": func() { props.OnMode(WorkMode.Duration) }, + "onClick": func() { props.OnMode(WorkMode) }, }, "Work Mode", ), @@ - vdom.H("button", + vdom.H("button", map[string]any{ "className": "flex-1 px-3 py-3 text-base border-none rounded bg-green-500 text-white cursor-pointer hover:bg-green-600 transition-colors duration-200", - "onClick": func() { props.OnMode(BreakMode.Duration) }, + "onClick": func() { props.OnMode(BreakMode) }, }, "Break Mode", ), @@ - changeMode := func(duration int) { + changeMode := func(m Mode) { isRunning.Set(false) isComplete.Set(false) - minutes.Set(duration) + minutes.Set(m.Duration) seconds.Set(0) - if duration == WorkMode.Duration { - mode.Set(WorkMode.Name) - } else { - mode.Set(BreakMode.Name) - } + mode.Set(m.Name) }Also applies to: 82-86, 88-94, 166-176
108-109: Unused local state (isComplete).isComplete is set but never read. Either render completion UI or remove to reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tsunami/demo/githubaction/app.go(1 hunks)tsunami/demo/pomodoro/app.go(1 hunks)tsunami/demo/pomodoro/static/tw.css(1 hunks)tsunami/demo/recharts/app.go(1 hunks)tsunami/demo/tsunamiconfig/app.go(1 hunks)tsunami/engine/asyncnotify.go(1 hunks)tsunami/prompts/system.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tsunami/demo/githubaction/app.go
- tsunami/demo/tsunamiconfig/app.go
- tsunami/demo/recharts/app.go
- tsunami/demo/pomodoro/static/tw.css
🧰 Additional context used
🧬 Code graph analysis (2)
tsunami/engine/asyncnotify.go (2)
tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/app/defaultclient.go (1)
SendAsyncInitiation(34-36)
tsunami/demo/pomodoro/app.go (4)
tsunami/app/defaultclient.go (1)
DefineComponent(15-17)tsunami/vdom/vdom.go (2)
H(91-100)IfElse(114-119)tsunami/engine/hooks.go (3)
UseSetAppTitle(154-160)UseLocal(68-81)UseRef(97-104)tsunami/app/hooks.go (4)
UseSetAppTitle(95-101)UseLocal(106-116)UseRef(35-43)UseTicker(166-180)
🪛 markdownlint-cli2 (0.17.2)
tsunami/prompts/system.md
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1
(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1
(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1
(MD010, no-hard-tabs)
255-255: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
260-260: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
541-541: Hard tabs
Column: 1
(MD010, no-hard-tabs)
545-545: Hard tabs
Column: 1
(MD010, no-hard-tabs)
546-546: Hard tabs
Column: 1
(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1
(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1
(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1
(MD010, no-hard-tabs)
551-551: Hard tabs
Column: 1
(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1
(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1
(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1
(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1
(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1
(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1
(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1
(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1
(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1
(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1
(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1
(MD010, no-hard-tabs)
565-565: Hard tabs
Column: 1
(MD010, no-hard-tabs)
567-567: Hard tabs
Column: 1
(MD010, no-hard-tabs)
568-568: Hard tabs
Column: 1
(MD010, no-hard-tabs)
569-569: Hard tabs
Column: 1
(MD010, no-hard-tabs)
570-570: Hard tabs
Column: 1
(MD010, no-hard-tabs)
572-572: Hard tabs
Column: 1
(MD010, no-hard-tabs)
573-573: Hard tabs
Column: 1
(MD010, no-hard-tabs)
574-574: Hard tabs
Column: 1
(MD010, no-hard-tabs)
575-575: Hard tabs
Column: 1
(MD010, no-hard-tabs)
576-576: Hard tabs
Column: 1
(MD010, no-hard-tabs)
577-577: Hard tabs
Column: 1
(MD010, no-hard-tabs)
578-578: Hard tabs
Column: 1
(MD010, no-hard-tabs)
580-580: Hard tabs
Column: 1
(MD010, no-hard-tabs)
581-581: Hard tabs
Column: 1
(MD010, no-hard-tabs)
582-582: Hard tabs
Column: 1
(MD010, no-hard-tabs)
583-583: Hard tabs
Column: 1
(MD010, no-hard-tabs)
584-584: Hard tabs
Column: 1
(MD010, no-hard-tabs)
585-585: Hard tabs
Column: 1
(MD010, no-hard-tabs)
586-586: Hard tabs
Column: 1
(MD010, no-hard-tabs)
588-588: Hard tabs
Column: 1
(MD010, no-hard-tabs)
589-589: Hard tabs
Column: 1
(MD010, no-hard-tabs)
590-590: Hard tabs
Column: 1
(MD010, no-hard-tabs)
591-591: Hard tabs
Column: 1
(MD010, no-hard-tabs)
592-592: Hard tabs
Column: 1
(MD010, no-hard-tabs)
594-594: Hard tabs
Column: 1
(MD010, no-hard-tabs)
595-595: Hard tabs
Column: 1
(MD010, no-hard-tabs)
596-596: Hard tabs
Column: 1
(MD010, no-hard-tabs)
597-597: Hard tabs
Column: 1
(MD010, no-hard-tabs)
598-598: Hard tabs
Column: 1
(MD010, no-hard-tabs)
599-599: Hard tabs
Column: 1
(MD010, no-hard-tabs)
600-600: Hard tabs
Column: 1
(MD010, no-hard-tabs)
601-601: Hard tabs
Column: 1
(MD010, no-hard-tabs)
602-602: Hard tabs
Column: 1
(MD010, no-hard-tabs)
673-673: Hard tabs
Column: 1
(MD010, no-hard-tabs)
674-674: Hard tabs
Column: 1
(MD010, no-hard-tabs)
675-675: Hard tabs
Column: 1
(MD010, no-hard-tabs)
676-676: Hard tabs
Column: 1
(MD010, no-hard-tabs)
677-677: Hard tabs
Column: 1
(MD010, no-hard-tabs)
678-678: Hard tabs
Column: 1
(MD010, no-hard-tabs)
679-679: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1091-1091: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1178-1178: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1180-1180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1181-1181: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1189-1189: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1190-1190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1191-1191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1195-1195: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1196-1196: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1197-1197: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1202-1202: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1203-1203: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1204-1204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1205-1205: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1206-1206: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1207-1207: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1208-1208: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1209-1209: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1210-1210: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1211-1211: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1212-1212: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1213-1213: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1214-1214: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1215-1215: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1216-1216: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1217-1217: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1218-1218: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1223-1223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1224-1224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1225-1225: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1226-1226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1227-1227: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1228-1228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1229-1229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1231-1231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1232-1232: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1233-1233: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1234-1234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1235-1235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1236-1236: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1237-1237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1238-1238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1240-1240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1241-1241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1242-1242: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1243-1243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1244-1244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1245-1245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1246-1246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1247-1247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1262-1262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1263-1263: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1264-1264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1265-1265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1266-1266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1267-1267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1268-1268: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1269-1269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1270-1270: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1271-1271: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1273-1273: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1274-1274: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1275-1275: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1277-1277: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1278-1278: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1279-1279: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1280-1280: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1281-1281: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1282-1282: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1284-1284: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1285-1285: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1286-1286: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1287-1287: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1288-1288: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1289-1289: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1290-1290: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1291-1291: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1292-1292: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1293-1293: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1294-1294: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1295-1295: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1296-1296: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1297-1297: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1298-1298: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1299-1299: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1300-1300: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1302-1302: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1303-1303: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1304-1304: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1305-1305: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1306-1306: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1307-1307: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1308-1308: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1309-1309: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1310-1310: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1311-1311: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1353-1353: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
tsunami/engine/rootelem.go (6)
382-410: Guard effect hook access and root nil before render to prevent panicsAdd bounds/nil checks around comp.Hooks[EffectIndex] and skip nil hooks; also guard r.Root/r.Root.Elem before calling render.
@@ - // first, run effect cleanups + // first, run effect cleanups for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - r.runEffectUnmount(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffectUnmount(work, hook) } // now run, new effects for _, work := range workQueue { comp := r.CompMap[work.WaveId] if comp == nil { continue } - hook := comp.Hooks[work.EffectIndex] - r.runEffect(work, hook) + if work.EffectIndex < 0 || work.EffectIndex >= len(comp.Hooks) { + continue + } + hook := comp.Hooks[work.EffectIndex] + if hook == nil { + continue + } + r.runEffect(work, hook) } // now check if we need a render renderIds := r.getAndClearRenderWork() if len(renderIds) > 0 { - r.render(r.Root.Elem, &r.Root, "root", opts) + if r.Root != nil && r.Root.Elem != nil { + r.render(r.Root.Elem, &r.Root, "root", opts) + } else { + log.Printf("RunWork: root not initialized; skipping render") + } }
34-47: Protect RefOperations with a mutex (race fix)RefOperations is accessed from multiple goroutines; add a lock field on RootElem.
type RootElem struct { @@ RefOperations []vdom.VDomRefOperation + refOpsLock sync.Mutex Client *ClientImpl }
445-453: Make QueueRefOp/GetRefOperations thread-safeLock around append/swap to avoid data races with concurrent readers/writers.
func (r *RootElem) QueueRefOp(op vdom.VDomRefOperation) { - r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Lock() + r.RefOperations = append(r.RefOperations, op) + r.refOpsLock.Unlock() } func (r *RootElem) GetRefOperations() []vdom.VDomRefOperation { - ops := r.RefOperations - r.RefOperations = nil - return ops + r.refOpsLock.Lock() + ops := r.RefOperations + r.RefOperations = nil + r.refOpsLock.Unlock() + return ops }
212-221: Avoid lock coupling in SetAtomVal (unlock before calling atom)Hold atom map lock only for lookup; then call atom.SetVal outside to prevent nested lock stalls/deadlocks.
func (r *RootElem) SetAtomVal(name string, val any) error { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[name] + r.atomLock.Lock() + atom, ok := r.Atoms[name] + r.atomLock.Unlock() if !ok { return fmt.Errorf("atom %q not found", name) } return atom.SetVal(val) }
230-233: Fix typo in error message (“cannot b nil” → “cannot be nil”)User-facing text.
- return fmt.Errorf("Component function cannot b nil") + return fmt.Errorf("Component function cannot be nil")
262-268: Prevent accidental component re-registrationReturn an error if a component name is already registered.
func (r *RootElem) RegisterComponent(name string, cfunc any) error { if err := validateCFunc(cfunc); err != nil { return err } - r.CFuncs[name] = cfunc + if _, exists := r.CFuncs[name]; exists { + return fmt.Errorf("component %q already registered", name) + } + r.CFuncs[name] = cfunc return nil }tsunami/app/atom.go (2)
35-45: sameRef: honor “Nil values return false” for pointersGuard nils before direct comparison to avoid true on two nil pointers.
case reflect.Ptr: - // direct comparison works for *T - return any(oldVal) == any(newVal) + // guard nils to honor "nil values return false" + if vOld.IsZero() || vNew.IsZero() { + return false + } + // direct comparison works for *T + return any(oldVal) == any(newVal)
109-123: Make SetFn atomic via engine.SetFnAtomVal; keep DeepCopy + mutation warningPrevents lost updates under concurrent writers and preserves safety semantics.
func (a Atom[T]) SetFn(fn func(T) T) { vc := engine.GetGlobalRenderContext() if vc != nil { logInvalidAtomSet(a.name) return } - currentVal := a.Get() - copiedVal := DeepCopy(currentVal) - newVal := fn(copiedVal) - a.Set(newVal) + err := a.client.Root.SetFnAtomVal(a.name, func(val any) any { + typedVal := util.GetTypedAtomValue[T](val, a.name) + copied := DeepCopy(typedVal) + result := fn(copied) + if sameRef(typedVal, result) { + logMutationWarning(a.name) + } + return result + }) + if err != nil { + log.Printf("Failed to SetFn atom %s: %v", a.name, err) + return + } + a.client.Root.AtomAddRenderWork(a.name) }tsunami/demo/cpuchart/app.go (2)
51-67: Treat collection failures as missing data, not 0%Use nil CPUUsage on error to avoid skewing averages.
func generateCPUDataPoint() CPUDataPoint { now := time.Now() cpuUsage, err := collectCPUUsage() - if err != nil { - log.Printf("Error collecting CPU usage: %v", err) - cpuUsage = 0 - } - - dataPoint := CPUDataPoint{ + var cpuPtr *float64 + if err != nil { + log.Printf("Error collecting CPU usage: %v", err) + } else { + cpuPtr = &cpuUsage + } + + dataPoint := CPUDataPoint{ Time: now.Unix(), - CPUUsage: &cpuUsage, // Convert to pointer + CPUUsage: cpuPtr, Timestamp: now.Format("15:04:05"), } - log.Printf("CPU Usage: %.2f%% at %s", cpuUsage, dataPoint.Timestamp) + if cpuPtr != nil { + log.Printf("CPU Usage: %.2f%% at %s", *cpuPtr, dataPoint.Timestamp) + } return dataPoint }
40-49: Avoid doubled sampling interval (let ticker drive cadence)cpu.Percent blocks ~1s when given time.Second; use 0 for non-blocking read.
func collectCPUUsage() (float64, error) { - percentages, err := cpu.Percent(time.Second, false) + percentages, err := cpu.Percent(0, false)tsunami/engine/hooks.go (2)
6-11: Import reflect for safe dependency comparisonsRequired by depsEqual fix below.
import ( "log" "strconv" + "reflect" "github.com/wavetermdev/waveterm/tsunami/vdom" )
113-123: Make depsEqual safe for uncomparable typesAvoid panics on maps/slices/funcs; handle nils and use identity or DeepEqual as needed.
-func depsEqual(deps1 []any, deps2 []any) bool { - if len(deps1) != len(deps2) { - return false - } - for i := range deps1 { - if deps1[i] != deps2[i] { - return false - } - } - return true -} +func depsEqual(deps1 []any, deps2 []any) bool { + if len(deps1) != len(deps2) { + return false + } + for i := range deps1 { + d1, d2 := deps1[i], deps2[i] + if (d1 == nil) != (d2 == nil) { + return false + } + if d1 == nil { // both nil + continue + } + t1, t2 := reflect.TypeOf(d1), reflect.TypeOf(d2) + if t1 != t2 { + return false + } + if t1.Comparable() { + if d1 != d2 { + return false + } + continue + } + v1, v2 := reflect.ValueOf(d1), reflect.ValueOf(d2) + switch v1.Kind() { + case reflect.Slice, reflect.Map, reflect.Chan, reflect.Func, reflect.Pointer: + if v1.IsNil() || v2.IsNil() { + if v1.IsNil() && v2.IsNil() { + continue + } + return false + } + if v1.Pointer() != v2.Pointer() { + return false + } + default: + if !reflect.DeepEqual(d1, d2) { + return false + } + } + } + return true +}
🧹 Nitpick comments (2)
tsunami/engine/rootelem.go (2)
184-200: Reduce lock contention in AtomAddRenderWorkCopy usedBy under atom lock and release before scheduling render work to avoid lock ordering with needsRenderLock.
func (r *RootElem) AtomAddRenderWork(atomName string) { - r.atomLock.Lock() - defer r.atomLock.Unlock() - - atom, ok := r.Atoms[atomName] + r.atomLock.Lock() + atom, ok := r.Atoms[atomName] if !ok { - return + r.atomLock.Unlock() + return } - usedBy := atom.GetUsedBy() - if len(usedBy) == 0 { - return - } - for _, compId := range usedBy { + usedBy := append([]string(nil), atom.GetUsedBy()...) + r.atomLock.Unlock() + if len(usedBy) == 0 { + return + } + for _, compId := range usedBy { r.addRenderWork(compId) } }
85-111: Optional: don’t hold atomLock while calling atom.GetVal in GetDataMap/GetConfigMapLookup atoms under r.atomLock, then read values outside to avoid nested locks across many atoms.
Also applies to: 99-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tsunami/app/atom.go(1 hunks)tsunami/demo/cpuchart/app.go(1 hunks)tsunami/engine/comp.go(1 hunks)tsunami/engine/hooks.go(1 hunks)tsunami/engine/render.go(1 hunks)tsunami/engine/rootelem.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tsunami/engine/render.go
- tsunami/engine/comp.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-23T06:09:05.334Z
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1807
File: pkg/wshrpc/wshremote/wshremote.go:718-726
Timestamp: 2025-01-23T06:09:05.334Z
Learning: In Go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.Field != nil`) is the idiomatic way to handle multiple nil checks. Avoid suggesting nested if statements as they make the code more verbose without adding value.
Applied to files:
tsunami/app/atom.go
🧬 Code graph analysis (4)
tsunami/engine/hooks.go (6)
tsunami/engine/rootelem.go (1)
RootElem(34-47)tsunami/engine/comp.go (1)
ComponentImpl(21-45)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/app/hooks.go (8)
UseRenderTs(59-65)UseId(49-55)UseLocal(106-116)UseVDomRef(21-29)UseRef(35-43)UseEffect(84-91)UseResync(71-77)UseSetAppTitle(95-101)tsunami/engine/atomimpl.go (1)
MakeAtomImpl(18-24)tsunami/vdom/vdom_types.go (2)
VDomRef(35-41)ObjectType_Ref(13-13)
tsunami/engine/rootelem.go (9)
tsunami/engine/comp.go (1)
ComponentImpl(21-45)tsunami/vdom/vdom_types.go (4)
VDomRefOperation(115-120)VDomEvent(65-75)VDomFunc(25-32)VDomRef(35-41)tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/engine/globalctx.go (3)
GlobalContextType_async(14-14)EventContextImpl(33-36)EffectContextImpl(38-42)tsunami/util/util.go (1)
PanicHandler(24-34)tsunami/engine/hooks.go (1)
Hook(14-21)tsunami/engine/render.go (1)
RenderOpts(19-21)tsunami/rpctypes/protocoltypes.go (1)
VDomRefUpdate(165-169)tsunami/app/defaultclient.go (1)
QueueRefOp(101-110)
tsunami/demo/cpuchart/app.go (4)
tsunami/app/defaultclient.go (3)
ConfigAtom(38-44)DataAtom(46-52)DefineComponent(15-17)tsunami/vdom/vdom.go (1)
H(91-100)tsunami/engine/hooks.go (1)
UseSetAppTitle(161-167)tsunami/app/hooks.go (2)
UseSetAppTitle(95-101)UseTicker(166-180)
tsunami/app/atom.go (4)
tsunami/engine/clientimpl.go (1)
ClientImpl(37-61)tsunami/engine/globalctx.go (1)
GetGlobalRenderContext(64-72)tsunami/util/util.go (1)
GetTypedAtomValue(103-121)tsunami/app/defaultclient.go (1)
DeepCopy(84-95)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
tsunami/engine/rootelem.go (1)
27-33: Incorrect — genAtom does not require SetFnVal; AtomImpl already satisfies the interface
genAtom (tsunami/engine/rootelem.go) declares GetVal, SetVal, SetUsedBy, GetUsedBy; AtomImpl[T] (tsunami/engine/atomimpl.go) implements those methods and no SetFnVal occurrences exist in the repo. No change required.Likely an incorrect or invalid review comment.
| type genAtom interface { | ||
| GetVal() any | ||
| SetVal(any) error | ||
| SetUsedBy(string, bool) | ||
| GetUsedBy() []string | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add atomic SetFnAtomVal and extend genAtom (prevents lost updates in SetFn)
Expose an engine-level read-modify-write to make SetFn atomic and safe.
type genAtom interface {
GetVal() any
SetVal(any) error
+ SetFnVal(func(any) any) error
SetUsedBy(string, bool)
GetUsedBy() []string
}+func (r *RootElem) SetFnAtomVal(name string, setFn func(any) any) error {
+ r.atomLock.Lock()
+ atom, ok := r.Atoms[name]
+ r.atomLock.Unlock()
+ if !ok {
+ return fmt.Errorf("atom %q not found", name)
+ }
+ return atom.SetFnVal(setFn)
+}Also applies to: 212-221
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
tsunami/build/build.go (10)
351-367: Fix misleading cleanup logging and keep behavior.Only log “keeping” when KeepTemp is true; don’t log when TempDir is empty.
be.cleanupOnce.Do(func() { - if keepTemp || be.TempDir == "" { - log.Printf("NOT cleaning tempdir\n") - return - } + if keepTemp { + if verbose { + log.Printf("Keeping temporary directory: %s", be.TempDir) + } + return + } + if be.TempDir == "" { + return + }
369-384: Make signal handler stoppable to prevent goroutine leaks.Return a stopper to call on normal completion.
-func setupSignalCleanup(buildEnv *BuildEnv, keepTemp, verbose bool) { +func setupSignalCleanup(buildEnv *BuildEnv, keepTemp, verbose bool) func() { if keepTemp { - return + return func() {} } sigChan := make(chan os.Signal, 1) + done := make(chan struct{}) signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) go func() { defer signal.Stop(sigChan) - sig := <-sigChan - if verbose { - log.Printf("Received signal %v, cleaning up temp directory", sig) - } - buildEnv.cleanupTempDir(keepTemp, verbose) - os.Exit(1) + select { + case sig := <-sigChan: + if verbose { + log.Printf("Received signal %v, cleaning up temp directory", sig) + } + buildEnv.cleanupTempDir(keepTemp, verbose) + os.Exit(1) + case <-done: + return + } }() + return func() { close(done) } }
699-704: Fix parameter name: we’re reading stderr, not stdout.Aligns with StderrPipe usage.
-func monitorAndOpenBrowser(stdout io.ReadCloser, verbose bool) { - defer stdout.Close() - - scanner := bufio.NewScanner(stdout) +func monitorAndOpenBrowser(stderr io.ReadCloser, verbose bool) { + defer stderr.Close() + + scanner := bufio.NewScanner(stderr)
95-123: Make Tailwind detection robust and non-interactive.Use npx --yes and parse the version from the full output; log the matched version. Prevents hangs and brittle first-line parsing.
- // Check Tailwind CSS version - tailwindCmd := exec.Command("npx", "@tailwindcss/cli") + // Check Tailwind CSS version (non-interactive) + tailwindCmd := exec.Command("npx", "--yes", "@tailwindcss/cli", "--version") tailwindOutput, err := tailwindCmd.CombinedOutput() if err != nil { return nil, fmt.Errorf("failed to run 'npx @tailwindcss/cli': %w", err) } - tailwindStr := strings.TrimSpace(string(tailwindOutput)) - lines := strings.Split(tailwindStr, "\n") - if len(lines) == 0 { - return nil, fmt.Errorf("no output from tailwindcss command") - } - - firstLine := lines[0] - if verbose { - log.Printf("Found %s", firstLine) - } - - // Check for v4 (format: "≈ tailwindcss v4.1.12") - tailwindRegex := regexp.MustCompile(`tailwindcss v(\d+)`) - tailwindMatches := tailwindRegex.FindStringSubmatch(firstLine) - if len(tailwindMatches) < 2 { - return nil, fmt.Errorf("unable to parse tailwindcss version from: %s", firstLine) - } - - majorVersion, err := strconv.Atoi(tailwindMatches[1]) - if err != nil || majorVersion != 4 { - return nil, fmt.Errorf("tailwindcss v4 required, found: %s", firstLine) - } + tailwindStr := strings.TrimSpace(string(tailwindOutput)) + // Extract semver and enforce major version 4 + verRe := regexp.MustCompile(`\b(\d+)(?:\.\d+){0,2}\b`) + m := verRe.FindStringSubmatch(tailwindStr) + if len(m) < 2 { + return nil, fmt.Errorf("unable to parse tailwindcss version from: %s", tailwindStr) + } + major, err := strconv.Atoi(m[1]) + if err != nil || major != 4 { + return nil, fmt.Errorf("tailwindcss v4 required, found: %s", tailwindStr) + } + if verbose { + log.Printf("Found tailwindcss %s", m[0]) + }
184-188: Validate SdkReplacePath and avoid duplicate replace directives.Adding an empty or duplicate replace will break mod parsing.
- // Add replace directive for tsunami SDK - if err := modFile.AddReplace("github.com/wavetermdev/waveterm/tsunami", "", opts.SdkReplacePath, ""); err != nil { - return fmt.Errorf("failed to add replace directive: %w", err) - } + // Validate and add/ensure replace directive for tsunami SDK + if opts.SdkReplacePath == "" { + return fmt.Errorf("SdkReplacePath cannot be empty") + } + if _, err := os.Stat(opts.SdkReplacePath); err != nil { + return fmt.Errorf("invalid SdkReplacePath %q: %w", opts.SdkReplacePath, err) + } + replaced := false + for _, r := range modFile.Replace { + if r.Old.Path == "github.com/wavetermdev/waveterm/tsunami" { + r.New.Path = opts.SdkReplacePath + r.New.Version = "" + replaced = true + break + } + } + if !replaced { + if err := modFile.AddReplace("github.com/wavetermdev/waveterm/tsunami", "", opts.SdkReplacePath, ""); err != nil { + return fmt.Errorf("failed to add replace directive: %w", err) + } + }
467-474: Verify UI target exists before creating symlink.Prevents broken symlinks when UI package is imported.
uiLinkPath := filepath.Join(tempDir, "ui") uiTargetPath := filepath.Join(opts.SdkReplacePath, "ui") - if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { + if _, err := os.Stat(uiTargetPath); err != nil { + return buildEnv, fmt.Errorf("UI target path does not exist at %s: %w", uiTargetPath, err) + } + if err := os.Symlink(uiTargetPath, uiLinkPath); err != nil { return buildEnv, fmt.Errorf("failed to create ui symlink: %w", err) }
594-602: Ensure temp/static exists before writing tw.css.Avoids failure when the app has no static/ yet.
// tailwind.css is already in tempDir from scaffold copy tailwindOutput := filepath.Join(tempDir, "static", "tw.css") + if err := os.MkdirAll(filepath.Dir(tailwindOutput), 0o755); err != nil { + return fmt.Errorf("failed to ensure static directory: %w", err) + } tailwindCmd := exec.Command("npx", "@tailwindcss/cli",
386-394: Avoid nil deref and coordinate cleanup in TsunamiBuild.Guard defer and stop the signal goroutine on success.
func TsunamiBuild(opts BuildOpts) error { - buildEnv, err := tsunamiBuildInternal(opts) - defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) - if err != nil { - return err - } - setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + buildEnv, err := tsunamiBuildInternal(opts) + if err != nil { + if buildEnv != nil { + buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + } + return err + } + defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + stopSignalHandler := setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + defer stopSignalHandler() return nil }
644-651: Avoid nil deref and coordinate cleanup in TsunamiRun.Same guard as build path.
func TsunamiRun(opts BuildOpts) error { - buildEnv, err := tsunamiBuildInternal(opts) - defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) - if err != nil { - return err - } - setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + buildEnv, err := tsunamiBuildInternal(opts) + if err != nil { + if buildEnv != nil { + buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + } + return err + } + defer buildEnv.cleanupTempDir(opts.KeepTemp, opts.Verbose) + stopSignalHandler := setupSignalCleanup(buildEnv, opts.KeepTemp, opts.Verbose) + defer stopSignalHandler()
652-654: Honor OutputFile when running the app.Run the binary where it was actually written.
// Run the built application - appPath := filepath.Join(buildEnv.TempDir, "bin", "app") + appPath := filepath.Join(buildEnv.TempDir, "bin", "app") + if opts.OutputFile != "" { + abs, err := filepath.Abs(opts.OutputFile) + if err != nil { + return fmt.Errorf("failed to resolve output path: %w", err) + } + appPath = abs + }
🧹 Nitpick comments (2)
tsunami/build/build.go (2)
726-732: Remove redundant second scan loop.Scanner’s stream is already exhausted; this never prints.
- // Continue reading and printing output if verbose - if verbose { - for scanner.Scan() { - fmt.Println(scanner.Text()) - } - }
285-293: Minor: avoid double stat checks in scaffold validation.You first call IsDirOrNotFound and then os.Stat + IsDir; one check is enough. Trim duplication for dist and node_modules.
Also applies to: 313-321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tsunami/build/build.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tsunami/build/build.go (2)
tsunami/build/buildutil.go (3)
CheckFileExists(26-40)IsDirOrNotFound(10-24)FileMustNotExist(42-49)tsunami/util/util.go (1)
OpenBrowser(80-101)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tsunami/demo/tsunamiconfig/static/tw.css (1)
1-3: Scope this Tailwind sheet to the tsunamiconfig demo (don’t inject globally).Loading this file for every Tsunami view will leak utilities and base resets across demos and cause ordering-specific conflicts. Gate the
<link>to only render for the tsunamiconfig demo, or prefix all rules via build-time scoping.Example (outside this file, in tsunami/frontend/src/vdom.tsx) — only include for the tsunamiconfig demo:
- <link rel="stylesheet" href={`/static/tw.css?x=${model.serverId}`} /> + {model.appId === 'tsunamiconfig' && ( + <link rel="stylesheet" href={`/static/tw.css?x=${model.serverId}`} /> + )}If you prefer CSS scoping at build-time, use a PostCSS prefixer (e.g., postcss-prefix-selector) to wrap all selectors with
:where(.tsunamiconfig-root).
🧹 Nitpick comments (5)
tsunami/demo/tsunamiconfig/static/tw.css (5)
4-61: Namespace theme tokens to avoid cross-app variable collisions.Variables like
--color-accent,--color-background, etc., on:rootwill bleed into other demos if this sheet loads anywhere else. Consider moving these under a theme class (e.g.,.tsunamiconfig-theme) and applying that class on the demo root.Minimal change inside this file:
-@layer theme { - :root, :host { +@layer theme { + :where(.tsunamiconfig-theme, :host(.tsunamiconfig-theme)) { /* vars... */ } }
62-209: Tailwind base/reset here affects global elements.The
@layer baserules alterhtml,a,button, etc. If scoping at CSS level (not via conditional link), consider building Tailwind withpreflight: falsefor this demo and include a single shared reset once, globally.
210-959: Utilities bundle is large; tree-shake to actual usage.This looks like the full v4 utility set. For demos, compile with content-aware extraction to ship only used classes, or reuse a shared core Tailwind build plus a tiny demo-specific layer.
Build-time suggestion (outside this file): configure Tailwind with
contentglobs for the tsunamiconfig demo paths and emit a minimized CSS for this demo only.
650-655: Verifymask-clip: no-clipcross-browser support.
no-cliphas patchy support. If this utility is unused, drop it; if used, provide a fallback or guard with@supports.Would you like a quick scan to confirm any in-repo usage of
.mask-no-clip?
960-1284: Registered custom properties: consider compatibility guard.The many
@property --tw-*registrations can trip older Safari versions. If you must support them, wrap in@supports (property(--a))or feature-detect where needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tsunami/demo/githubaction/static/tw.css(1 hunks)tsunami/demo/tabletest/static/tw.css(1 hunks)tsunami/demo/tsunamiconfig/static/tw.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsunami/demo/tabletest/static/tw.css
🚧 Files skipped from review as they are similar to previous changes (1)
- tsunami/demo/githubaction/static/tw.css
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tsunami/demo/pomodoro/app.go (3)
114-137: Avoid goroutine churn: keep a single ticker and guard with isRunning.Passing []any{isRunning.Get()} as deps likely tears down and recreates the ticker goroutine on every state flip. Since the tickFn already early-returns when not running, prefer no deps to reduce churn and potential leaks if cancellation ever misfires. Verify UseGoRoutine cancels on dep changes if you keep this pattern.
- }, []any{isRunning.Get()}) + }, nil)
20-22: Namespace the public data atom to avoid collisions across demos/apps.DataAtom is global; a generic name like "remainingSeconds" can collide if multiple apps run in one client. Prefix it (e.g., "pomodoro.remainingSeconds").
- remainingSecondsAtom = app.DataAtom("remainingSeconds", WorkMode.Duration*60) + remainingSecondsAtom = app.DataAtom("pomodoro.remainingSeconds", WorkMode.Duration*60)
29-35: Drop JSON tags on function props; they aren’t serializable and can confuse tooling.Function-typed fields won’t be JSON-encoded by design; keeping tags is misleading.
type ControlButtonsProps struct { - IsRunning bool `json:"isRunning"` - OnStart func() `json:"onStart"` - OnPause func() `json:"onPause"` - OnReset func() `json:"onReset"` - OnMode func(int) `json:"onMode"` + IsRunning bool `json:"isRunning"` + OnStart func() + OnPause func() + OnReset func() + OnMode func(int) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tsunami/demo/pomodoro/app.go(1 hunks)tsunami/demo/pomodoro/static/tw.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsunami/demo/pomodoro/static/tw.css
🧰 Additional context used
🧬 Code graph analysis (1)
tsunami/demo/pomodoro/app.go (4)
tsunami/app/defaultclient.go (2)
DataAtom(46-52)DefineComponent(15-17)tsunami/vdom/vdom.go (2)
H(91-100)IfElse(114-119)tsunami/engine/hooks.go (3)
UseSetAppTitle(161-167)UseLocal(75-88)UseRef(104-111)tsunami/app/hooks.go (4)
UseSetAppTitle(95-101)UseLocal(106-116)UseRef(35-43)UseTicker(166-180)
⏰ 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). (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
tsunami/demo/pomodoro/app.go (1)
38-54: TimerDisplay formatting looks solid.
| pauseTimer := func() { | ||
| if !isRunning.Get() { | ||
| return | ||
| } | ||
|
|
||
| // Calculate remaining time and update remainingSeconds | ||
| elapsed := time.Since(startTime.Current) | ||
| remaining := totalDuration.Current - elapsed | ||
| if remaining > 0 { | ||
| remainingSecondsAtom.Set(int(remaining.Seconds())) | ||
| } | ||
| isRunning.Set(false) | ||
| } |
There was a problem hiding this comment.
Fix pause-at-expiry edge case: ensure completion and 0s are set.
If Pause is clicked right as the timer expires, remaining can be <= 0 but we don't set remainingSeconds to 0 or mark complete. The ticker stops (isRunning=false) and the UI can get stuck with a nonzero display and not-complete state.
Apply:
// Calculate remaining time and update remainingSeconds
elapsed := time.Since(startTime.Current)
remaining := totalDuration.Current - elapsed
- if remaining > 0 {
- remainingSecondsAtom.Set(int(remaining.Seconds()))
- }
- isRunning.Set(false)
+ if remaining <= 0 {
+ remainingSecondsAtom.Set(0)
+ isComplete.Set(true)
+ isRunning.Set(false)
+ return
+ }
+ remainingSecondsAtom.Set(int(remaining.Seconds()))
+ isRunning.Set(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.
| pauseTimer := func() { | |
| if !isRunning.Get() { | |
| return | |
| } | |
| // Calculate remaining time and update remainingSeconds | |
| elapsed := time.Since(startTime.Current) | |
| remaining := totalDuration.Current - elapsed | |
| if remaining > 0 { | |
| remainingSecondsAtom.Set(int(remaining.Seconds())) | |
| } | |
| isRunning.Set(false) | |
| } | |
| pauseTimer := func() { | |
| if !isRunning.Get() { | |
| return | |
| } | |
| // Calculate remaining time and update remainingSeconds | |
| elapsed := time.Since(startTime.Current) | |
| remaining := totalDuration.Current - elapsed | |
| if remaining <= 0 { | |
| remainingSecondsAtom.Set(0) | |
| isComplete.Set(true) | |
| isRunning.Set(false) | |
| return | |
| } | |
| remainingSecondsAtom.Set(int(remaining.Seconds())) | |
| isRunning.Set(false) | |
| } |
🤖 Prompt for AI Agents
In tsunami/demo/pomodoro/app.go around lines 150-162, the pauseTimer handler
doesn't handle the edge case where remaining <= 0: when Pause is clicked right
as the timer expires we must set remainingSeconds to 0 and mark the timer
complete. Update the branch so that if remaining <= 0 you call
remainingSecondsAtom.Set(0), set the completion flag (isComplete.Set(true) — or
the equivalent completion atom in this file), and then set isRunning.Set(false);
otherwise keep the existing behavior of setting remainingSecondsAtom to the
computed seconds and stopping the run.
1 participant
No description provided.