mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-08-29 03:44:06 +08:00
fix(tui): preserve SolidJS prop reactivity in Slot wrapper
Replace object spread (`{...props}`) with `mergeProps` when forwarding
props through the plugin Slot wrapper. Spreading props in SolidJS
evaluates each prop once at mount time, freezing reactive values like
`ref`, `visible`, `disabled`, and `on_submit`. This caused handlers to
bind against stale closures after overlay transitions, breaking Enter
submission on the session_prompt slot.
Add regression tests verifying the wrapper uses `mergeProps` (not
spread) and that reactive prop tracking is preserved at runtime.
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
import type { TuiPluginApi, TuiSlotContext, TuiSlotMap, TuiSlotProps } from "@kilocode/plugin/tui"
|
||||
import { createSlot, createSolidSlotRegistry, type JSX, type SolidPlugin } from "@opentui/solid"
|
||||
import { children } from "solid-js" // kilocode_change
|
||||
import { children, mergeProps } from "solid-js" // kilocode_change
|
||||
import { isRecord } from "@/util/record"
|
||||
|
||||
type RuntimeSlotMap = TuiSlotMap<Record<string, object>>
|
||||
@@ -24,14 +24,16 @@ let view: Slot = empty
|
||||
|
||||
// kilocode_change start - stabilize fallback children so replace-mode slots
|
||||
// don't recreate stateful defaults like the session prompt on prop changes.
|
||||
// mergeProps (instead of spread) preserves SolidJS prop reactivity so things
|
||||
// like ref, visible, disabled, on_submit keep flowing through to the slot.
|
||||
export const Slot = <Name extends string>(props: TuiSlotProps<Name>) => {
|
||||
const value = children(() => props.children)
|
||||
return view({
|
||||
...props,
|
||||
const merged = mergeProps(props, {
|
||||
get children() {
|
||||
return value()
|
||||
},
|
||||
} as TuiSlotProps<Name>)
|
||||
})
|
||||
return view(merged as TuiSlotProps<Name>)
|
||||
}
|
||||
// kilocode_change end
|
||||
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
/**
|
||||
* Regression test for the Slot wrapper in plugin/slots.tsx.
|
||||
*
|
||||
* PR #9425 introduced a wrapper around the opentui Slot that memoizes the
|
||||
* fallback children so stateful defaults (like the session prompt) aren't
|
||||
* recreated when props change. The first implementation used an object spread
|
||||
* `{...props}` to forward props, which *breaks SolidJS prop reactivity*: every
|
||||
* prop except the getter-defined `children` freezes at mount time.
|
||||
*
|
||||
* Downstream consequence: `ref`, `visible`, `disabled`, `on_submit` stop
|
||||
* updating on the session_prompt slot once the outer <Show> no longer gates
|
||||
* mounting. That's what made Enter stop submitting after a blocking overlay
|
||||
* closed — the prompt ref callback and submit handler were captured against
|
||||
* stale closures.
|
||||
*
|
||||
* This test locks in two things:
|
||||
* 1. A static invariant: the wrapper does NOT spread raw props (`...props`),
|
||||
* which would silently reintroduce the regression on refactors.
|
||||
* 2. A runtime check: forwarding props through the same pattern used in
|
||||
* slots.tsx preserves reactivity for arbitrary props (not just children).
|
||||
*/
|
||||
|
||||
import { describe, expect, test } from "bun:test"
|
||||
import fs from "node:fs"
|
||||
import path from "node:path"
|
||||
import { children, createEffect, createRoot, createSignal, mergeProps } from "solid-js"
|
||||
|
||||
const SLOTS_FILE = path.resolve(import.meta.dir, "../../src/cli/cmd/tui/plugin/slots.tsx")
|
||||
|
||||
describe("Slot wrapper preserves prop reactivity", () => {
|
||||
test("slots.tsx does not use `{...props}` spread to forward props", () => {
|
||||
// Spread on a plain object in Solid evaluates every prop once and freezes
|
||||
// it. mergeProps (or a getter per prop) is required to keep reactivity.
|
||||
const content = fs.readFileSync(SLOTS_FILE, "utf-8")
|
||||
const wrapper = content.match(/export const Slot[\s\S]*?^}/m)?.[0] ?? ""
|
||||
expect(wrapper).not.toBe("")
|
||||
expect(wrapper).not.toMatch(/\.\.\.props/)
|
||||
})
|
||||
|
||||
test("slots.tsx forwards props through mergeProps (or per-prop getters)", () => {
|
||||
const content = fs.readFileSync(SLOTS_FILE, "utf-8")
|
||||
const wrapper = content.match(/export const Slot[\s\S]*?^}/m)?.[0] ?? ""
|
||||
const usesMergeProps = /mergeProps\s*\(/.test(wrapper)
|
||||
expect(usesMergeProps).toBe(true)
|
||||
})
|
||||
|
||||
test("mergeProps preserves reactivity of non-children props", () => {
|
||||
// Simulates the exact pattern used in slots.tsx: resolve children via the
|
||||
// `children()` helper and forward the rest via mergeProps. Non-children
|
||||
// reactive props (like `visible`, `disabled`, `ref`) must keep tracking
|
||||
// their source signals — otherwise the slot-internal consumer (opentui
|
||||
// registry → plugin) sees a frozen initial value.
|
||||
const [visible, setVisible] = createSignal(true)
|
||||
const [disabled, setDisabled] = createSignal(false)
|
||||
const refCalls: Array<string> = []
|
||||
const refA = () => refCalls.push("a")
|
||||
const refB = () => refCalls.push("b")
|
||||
const [ref, setRef] = createSignal<() => void>(refA)
|
||||
|
||||
const seen: Array<{ visible: boolean; disabled: boolean }> = []
|
||||
const refSeen: Array<() => void> = []
|
||||
|
||||
const dispose = createRoot((dispose) => {
|
||||
// Pretend JSX: reactive props passed into the Slot wrapper.
|
||||
const sourceProps = {
|
||||
get visible() {
|
||||
return visible()
|
||||
},
|
||||
get disabled() {
|
||||
return disabled()
|
||||
},
|
||||
get ref() {
|
||||
return ref()
|
||||
},
|
||||
children: "unused",
|
||||
}
|
||||
|
||||
// This mirrors plugin/slots.tsx exactly.
|
||||
const value = children(() => sourceProps.children)
|
||||
const merged = mergeProps(sourceProps, {
|
||||
get children() {
|
||||
return value()
|
||||
},
|
||||
}) as typeof sourceProps
|
||||
|
||||
createEffect(() => {
|
||||
seen.push({ visible: merged.visible, disabled: merged.disabled })
|
||||
})
|
||||
createEffect(() => {
|
||||
refSeen.push(merged.ref)
|
||||
})
|
||||
|
||||
return dispose
|
||||
})
|
||||
|
||||
// Initial render tracked.
|
||||
expect(seen).toEqual([{ visible: true, disabled: false }])
|
||||
expect(refSeen.length).toBe(1)
|
||||
expect(refSeen[0]).toBe(refA)
|
||||
|
||||
// Flip the source signals — the merged view must update.
|
||||
setVisible(false)
|
||||
expect(seen).toEqual([
|
||||
{ visible: true, disabled: false },
|
||||
{ visible: false, disabled: false },
|
||||
])
|
||||
|
||||
setDisabled(true)
|
||||
expect(seen[seen.length - 1]).toEqual({ visible: false, disabled: true })
|
||||
|
||||
// Ref callback must also track through the wrapper — this is what makes
|
||||
// the session prompt ref={bind} actually attach/re-attach correctly.
|
||||
setRef(() => refB)
|
||||
expect(refSeen.length).toBe(2)
|
||||
expect(refSeen[1]).toBe(refB)
|
||||
|
||||
dispose()
|
||||
})
|
||||
|
||||
test("plain `{...props}` spread does NOT preserve reactivity (proves the regression)", () => {
|
||||
// Negative control: the exact bug we're guarding against. A spread into a
|
||||
// plain object decouples the reactive source, so an effect on the copy
|
||||
// only fires once.
|
||||
const [visible, setVisible] = createSignal(true)
|
||||
let fires = 0
|
||||
|
||||
const dispose = createRoot((dispose) => {
|
||||
const sourceProps = {
|
||||
get visible() {
|
||||
return visible()
|
||||
},
|
||||
}
|
||||
|
||||
// BUG pattern — copies the value at evaluation time.
|
||||
const frozen = { ...sourceProps } as { visible: boolean }
|
||||
|
||||
createEffect(() => {
|
||||
// Touch frozen.visible to subscribe (but it's a static property now).
|
||||
void frozen.visible
|
||||
fires++
|
||||
})
|
||||
|
||||
return dispose
|
||||
})
|
||||
|
||||
expect(fires).toBe(1)
|
||||
setVisible(false)
|
||||
// A correctly reactive wrapper would have fired again; the frozen copy
|
||||
// does not. Keeping this assertion documents why mergeProps is required.
|
||||
expect(fires).toBe(1)
|
||||
|
||||
dispose()
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user