Refactor to Declarative Component System & Color Spruce-up#14
Open
dylanwatsonsoftware wants to merge 3 commits into
Open
Refactor to Declarative Component System & Color Spruce-up#14dylanwatsonsoftware wants to merge 3 commits into
dylanwatsonsoftware wants to merge 3 commits into
Conversation
- Introduce central `Boba.run` loop for unified rendering and event handling - Refactor `BobaComponent` with `onEvent` and `tick` lifecycle methods - Update layout components to propagate coordinates and events to children - Port interactive components to the new declarative model - Spruce up `ConsoleColors` with Bubble Tea-inspired RGB palette - Migrate sample and WASM apps to the new API Co-authored-by: dylanwatsonsoftware <2309644+dylanwatsonsoftware@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR converts Bobatea from suspend-loop-driven components to a component/event-driven architecture: adds per-component layout state and lifecycle hooks, introduces a central Boba.run render–event–tick loop, makes Terminal.readEvent timeout/nullable, and updates many components to use onEvent/tick and report measured width/height. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BobaRun as Boba.run()
participant Terminal
participant Root as RootComponent
participant Child as ChildComponent
loop Render-Tick Loop
BobaRun->>Terminal: size()
BobaRun->>Root: render(width,height)
Root->>Child: render(availableWidth)
Child-->>Root: rendered string + layout (x,y,widthPx,heightPx)
Root-->>BobaRun: composed output
BobaRun->>Terminal: clear & write output
BobaRun->>Terminal: readEvent(50)
alt Event received
Terminal-->>BobaRun: BobaEvent
BobaRun->>Root: onEvent(event)
Root->>Child: onEvent(event)
alt Child handles event
Child-->>Root: true
else
Child-->>Root: false
end
else Timeout (null)
Terminal-->>BobaRun: null
end
BobaRun->>Root: tick(deltaMs)
Root->>Child: tick(deltaMs)
BobaRun->>BobaRun: delay(10)
end
User->>Terminal: keyboard / mouse input
Terminal-->>User: visual update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt (1)
28-40:⚠️ Potential issue | 🟠 Major
runLoadingno longer shows a loader.This public API now just delegates to
callback()and ignoresterminal, styling, and layout arguments, so existing callers never render a spinner.bobatea/src/jvmTest/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaTest.ktstill exercises this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt` around lines 28 - 40, The runLoading function currently bypasses rendering and simply returns callback(); restore its original behavior by implementing a suspendable loader lifecycle around the callback: use the runLoading(<message>, style: LoaderStyle, terminal: Terminal, padding: Int, margin: Int, borderStyle: BorderStyle, color: String?, callback: suspend () -> R) signature to start a spinner/loader on the provided Terminal (respecting LoaderStyle, padding, margin, borderStyle and color), run the callback, and ensure the loader is stopped/cleared afterwards even on exceptions, then return the callback result; locate runLoading in LoadingIndicator.kt and wrap callback execution with start/stop logic so tests in BobaTest.kt that exercise this path will see the spinner.
🧹 Nitpick comments (2)
bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt (1)
57-61: Keep the leftover elapsed time intick().Resetting
elapsedMsto0drops any extra delay, so long frames make the animation run slower than intended.Suggested fix
override fun tick(deltaMs: Long) { elapsedMs += deltaMs - if (elapsedMs >= 200) { + while (elapsedMs >= 200) { frameIndex++ - elapsedMs = 0 + elapsedMs -= 200 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt` around lines 57 - 61, In tick(deltaMs: Long) the code currently sets elapsedMs = 0 which discards any leftover time; change the logic in LoadingIndicator.tick to subtract the frame duration (200 ms) from elapsedMs (and loop/subtract repeatedly if elapsedMs >= 200) when advancing frameIndex so leftover time carries over instead of being lost; update references to elapsedMs, frameIndex and the 200 ms frame duration in that method accordingly.bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.kt (1)
33-37: Use the rendered bounds for the back button hitbox.The current
textLength = 16check already accepts one extra column because of<=, and it will drift if the label or wrapping changes. The button already measures itself inrender(), so it would be safer to derive the clickable area from that state instead of a magic width.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.kt` around lines 33 - 37, The hitbox uses a hardcoded textLength (16) which will drift; instead use the actual rendered width computed in render() (e.g., a measuredWidth / labelWidth field set by BackButton.render) to determine the clickable X range and replace the magic constant with that field in the click check (use the same border/padding offsets as already used). Update the comparison to use the rendered width (event.x < x + ... + renderedWidth) so the hitbox always matches the drawn button; reference BackButton.render and the BackButton click handling logic where textLength is currently used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt`:
- Around line 52-53: The hit-test for the title uses hard-coded x bounds and
title length which ignores the same inset used vertically; update the
hit-testing logic in ExpandableComponent.kt to compute the title X origin the
same way as titleLine (use borderStyle and padding to compute an xInset/left
offset) and compute the hitbox width from the rendered title string (e.g.,
measure title.length or the rendered width) instead of "title.length + 4"; then
compare event.x against that derived xOrigin and xOrigin + titleWidth so
hover/click align with the displayed title. Ensure you update both the
mouse-check branch(s) that reference x and title.length (including the
comparisons around event and title) to use the new xInset and measured title
width.
- Around line 54-58: The handler for BobaEvent.Key currently toggles expanded on
any SPACE/ENTER and so the first ExpandableComponent steals those keys; change
the condition in the BobaEvent.Key branch so you only toggle expanded when this
component is the intended target (e.g., the component has focus or the event is
routed to it) instead of unconditionally. Concretely, wrap the
SPACE.key/ENTER.key check with a focus/route guard (check your component's focus
state method or event target/routing flag on the incoming BobaEvent.Key) before
flipping expanded so later siblings aren't preempted (symbols to modify:
BobaEvent.Key handling, SPACE.key, ENTER.key, expanded; behavior referenced in
Stack.kt children propagation).
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Inline.kt`:
- Around line 42-52: In the Inline rendering loop (the children.forEach block in
Inline.kt) set the child's position (child.x and child.y) before invoking
child.render(...) so nested containers compute correct coordinates for their
descendants; move the assignment of child.x = currentX and child.y =
this@Inline.y + padding + border offset to precede the call to
child.render(childAvailableWidth, resolvedHeight), then compute
child.widthPx/heightPx from the rendered output and advance currentX and emit
the cell(Text(renderedChild)) as before.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`:
- Around line 60-77: The onEvent handler in MultiSelectionList uses modulo
arithmetic and toggle/currentIndex access without guarding empty options, which
crashes when options.isEmpty(); fix by short-circuiting when options.isEmpty()
(e.g., at the top of onEvent return false or handle Enter/Q accordingly) or
enforce non-empty lists in the MultiSelectionList constructor (validate options
and throw/require). Specifically, modify MultiSelectionList.onEvent to check
options.isEmpty() before any use of currentIndex, options.size,
toggle(currentIndex) or reading selected, or add a constructor check for
options.isEmpty() and reject/throw to ensure callers cannot pass an empty list.
- Around line 82-89: The mouse handler in MultiSelectionList (inside the
BobaEvent.Mouse branch) assumes one row per option and ignores event.x, causing
incorrect toggles for wrapped options or clicks outside the component; update
the handler to compute the component's top Y (use the same borderStyle/padding
logic already used for rendering), compute each option's rendered height
(accounting for wrapping/line breaks and any per-option rowHeight), build a
mapping from screen Y ranges to option indices, and only toggle when event.x
falls within the component's left/right bounds and event.y maps to a specific
option; modify the BobaEvent.Mouse logic that sets
startLine/clickedIndex/currentIndex and calls toggle(currentIndex) to use this
bounds-checked, wrap-aware mapping instead.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.kt`:
- Around line 46-60: SelectionList.onEvent currently performs modulo by
options.size and indexes options[currentIndex] which will crash when options is
empty; either validate/reject empty lists when constructing SelectionList (throw
IllegalArgumentException in constructor) or add an early guard at the top of
onEvent (if options.isEmpty() return false or handle Up/Down/Confirm as no-ops)
so navigation and confirm branches never compute % options.size or access
options[currentIndex]; update references to currentIndex, options, and onSelect
accordingly to ensure they are only used when options.isNotEmpty().
- Around line 65-76: The mouse hit-test assumes one-row-per-item and ignores
horizontal bounds; replace the simple clickedIndex = event.y - startLine logic
in the BobaEvent.Mouse branch with a proper per-option bounds check: compute the
actual top and bottom Y for each option by accounting for the prompt height,
borderStyle, padding, and the measured/wrapped height of each option (use
whatever layout/measure function or stored line counts your rendering path
provides), and compute the left/right X bounds from component X, padding and any
scroll/wrap offsets; then iterate options to find the one where event.y is
between its top and bottom and event.x is within its X bounds, only then update
currentIndex or call onSelect(selected); ignore clicks outside those bounds so
clicks outside the component do not select items.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Stack.kt`:
- Around line 36-46: The child coordinates are being set after calling
child.render, causing nested children to use stale x/y; move the coordinate
assignments so child.x and child.y (computed using this@Stack.x,
this@Stack.padding, borderStyle check, and currentY) are set before invoking
child.render(innerAvailableWidth, resolvedHeight), then compute renderedChild,
lines, widthPx and heightPx, update currentY, and finally call
row(Text(renderedChild)) so hit detection and descendant layout use the correct
positions.
In
`@bobatea/src/jvmMain/kotlin/com/github/dylanwatsonsoftware/bobatea/JvmTerminal.kt`:
- Around line 82-90: readEventWithTimeout currently returns readEventBlocking as
soon as one byte is available which allows the escape-sequence parser
(readEventBlocking / whatever internal parseEscapeSequence function handles
CSI/ESC sequences) to block indefinitely if only a partial sequence is present;
change readEventWithTimeout to propagate the deadline (compute deadline =
startTime + timeoutMs) into the parser by calling a new/overloaded parse
function that accepts the deadline, or modify readEventBlocking to accept a
deadline parameter and perform non-blocking reads until the deadline, and if the
deadline elapses while parsing an escape sequence return a standalone ESC event
(or the best-effort partial event) instead of blocking. Ensure the unique
symbols affected are readEventWithTimeout, readEventBlocking and the
escape-sequence parsing routine so the timeout budget is honored end-to-end.
In `@sample/src/main/kotlin/com/example/App.kt`:
- Around line 79-88: The SelectionList and MultiSelectionList are recreated
every render inside render(), and events are only forwarded to backButton so the
lists never receive input and their cursor state resets; fix by making the
SelectionList and MultiSelectionList persistent (create and store instances
outside render or keep them in App state) instead of rebuilding them each frame
and update render() to forward input/events to those stored instances as well as
to backButton (ensure Screen.SINGLE_SELECT and Screen.MULTI_SELECT branches
reference the persisted SelectionList/MultiSelectionList objects and not freshly
constructed ones).
- Line 105: CoordinatesDemo currently consumes every key event: change the
CoordinatesDemo.onEvent(event) implementation so it returns true only when it
actually handles an input (e.g., an arrow key moved the cursor) and returns
false for unhandled keys; this ensures the Screen.COORDINATES branch expression
(coordinatesDemo.onEvent(event) || backButton.onEvent(event)) allows
backButton.onEvent(event) to run for keys like 'q'. Update the onEvent method in
CoordinatesDemo (and the same logic in the block covering lines ~183–191) to
detect handled keys explicitly and return false otherwise.
- Around line 68-69: The render() method currently calls
kotlin.system.exitProcess(0) when App.currentScreen == Screen.EXIT which
bypasses main()'s finally block and prevents terminal.teardown() from running;
remove that hard exit and instead signal the run loop to stop—either change
render() to return an exit flag or set a shared state that Boba.run() observes
(use App.currentScreen == Screen.EXIT) and have Boba.run() break its event loop
so main()'s finally executes; ensure no direct calls to
kotlin.system.exitProcess(0) remain in render() and that Boba.run() performs the
clean shutdown.
In `@web/src/wasmJsMain/kotlin/Main.kt`:
- Line 66: WasmScreen.EXPANDABLE currently constructs a new ExpandableComponent
each render by calling buildWasmExpandable() twice; create and reuse a single
ExpandableComponent instance instead: instantiate buildWasmExpandable() once
(e.g., val expandable = buildWasmExpandable()) and pass that same expandable
into the Stack and any event handlers rather than calling buildWasmExpandable()
again so the ExpandableComponent instance retains its geometry/state across
renders; update references in the WasmScreen.EXPANDABLE branch (and the
corresponding similar branch at lines ~99) to use the shared expandable variable
and remove duplicate calls.
- Around line 68-75: The SelectionList and MultiSelectionList are being
recreated every render in the WasmScreen.SELECTION and WasmScreen.MULTI branches
which resets their internal state and prevents navigation; instead create and
retain stable instances (or initialize them only when the screen is entered) and
reuse them across frames: move creation of SelectionList and MultiSelectionList
out of the per-frame render branches into persistent properties (or lazily
initialize when currentWasmScreen transitions to WasmScreen.SELECTION /
WasmScreen.MULTI), keep the existing onSelect/onComplete callbacks to set
currentWasmScreen, and continue forwarding events to backButton as before.
- Around line 150-158: In onEvent(event: BobaEvent): Boolean (Main.kt), only
consume the key event when it actually moves the cursor: compute a local flag
(e.g., moved = false) and set it true inside the
KeyCodes.isDown/isUp/isLeft/isRight branches only when xPos or yPos is changed
(for example, when yPos < 7 then yPos++; moved = true). After the when block
return moved instead of unconditionally returning true so non-movement keys
(like 'q') propagate to backButton.onEvent(event).
---
Outside diff comments:
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt`:
- Around line 28-40: The runLoading function currently bypasses rendering and
simply returns callback(); restore its original behavior by implementing a
suspendable loader lifecycle around the callback: use the runLoading(<message>,
style: LoaderStyle, terminal: Terminal, padding: Int, margin: Int, borderStyle:
BorderStyle, color: String?, callback: suspend () -> R) signature to start a
spinner/loader on the provided Terminal (respecting LoaderStyle, padding,
margin, borderStyle and color), run the callback, and ensure the loader is
stopped/cleared afterwards even on exceptions, then return the callback result;
locate runLoading in LoadingIndicator.kt and wrap callback execution with
start/stop logic so tests in BobaTest.kt that exercise this path will see the
spinner.
---
Nitpick comments:
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.kt`:
- Around line 33-37: The hitbox uses a hardcoded textLength (16) which will
drift; instead use the actual rendered width computed in render() (e.g., a
measuredWidth / labelWidth field set by BackButton.render) to determine the
clickable X range and replace the magic constant with that field in the click
check (use the same border/padding offsets as already used). Update the
comparison to use the rendered width (event.x < x + ... + renderedWidth) so the
hitbox always matches the drawn button; reference BackButton.render and the
BackButton click handling logic where textLength is currently used.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.kt`:
- Around line 57-61: In tick(deltaMs: Long) the code currently sets elapsedMs =
0 which discards any leftover time; change the logic in LoadingIndicator.tick to
subtract the frame duration (200 ms) from elapsedMs (and loop/subtract
repeatedly if elapsedMs >= 200) when advancing frameIndex so leftover time
carries over instead of being lost; update references to elapsedMs, frameIndex
and the 200 ms frame duration in that method accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 004dc7ba-fe3b-49b6-9fea-00a18dcd485d
📒 Files selected for processing (19)
bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaComponent.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ConsoleColors.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Inline.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/LoadingIndicator.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Stack.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Terminal.ktbobatea/src/jvmMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.jvm.ktbobatea/src/jvmMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.ktbobatea/src/jvmMain/kotlin/com/github/dylanwatsonsoftware/bobatea/JvmTerminal.ktbobatea/src/jvmTest/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaTest.ktbobatea/src/wasmJsMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.wasm.ktbobatea/src/wasmJsMain/kotlin/com/github/dylanwatsonsoftware/bobatea/WasmTerminal.ktsample/src/main/kotlin/com/example/App.ktweb/src/wasmJsMain/kotlin/Main.kt
💤 Files with no reviewable changes (2)
- bobatea/src/jvmTest/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaTest.kt
- bobatea/src/jvmMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.kt
Comment on lines
+54
to
+58
| is BobaEvent.Key -> { | ||
| when (event.code) { | ||
| SPACE.key, ENTER.key -> { | ||
| expanded = !expanded | ||
| return true |
There was a problem hiding this comment.
Gate SPACE/ENTER behind focus or parent routing.
Line 56 toggles on every SPACE/ENTER event. In bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Stack.kt:59-63, children are tried in order and propagation stops on the first true, so the first ExpandableComponent in a container will steal those keys from later interactive siblings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt`
around lines 54 - 58, The handler for BobaEvent.Key currently toggles expanded
on any SPACE/ENTER and so the first ExpandableComponent steals those keys;
change the condition in the BobaEvent.Key branch so you only toggle expanded
when this component is the intended target (e.g., the component has focus or the
event is routed to it) instead of unconditionally. Concretely, wrap the
SPACE.key/ENTER.key check with a focus/route guard (check your component's focus
state method or event target/routing flag on the incoming BobaEvent.Key) before
flipping expanded so later siblings aren't preempted (symbols to modify:
BobaEvent.Key handling, SPACE.key, ENTER.key, expanded; behavior referenced in
Stack.kt children propagation).
Comment on lines
42
to
+52
| children.forEach { child -> | ||
| cell(Text(child.render(childAvailableWidth, resolvedHeight))) | ||
| val renderedChild = child.render(childAvailableWidth, resolvedHeight) | ||
| val lines = renderedChild.lines() | ||
|
|
||
| child.x = currentX | ||
| child.y = this@Inline.y + this@Inline.padding + (if (this@Inline.borderStyle != BorderStyle.NONE) 1 else 0) | ||
| child.widthPx = lines.maxOfOrNull { visibleLength(it) } ?: 0 | ||
| child.heightPx = lines.size | ||
|
|
||
| currentX += child.widthPx | ||
| cell(Text(renderedChild)) |
There was a problem hiding this comment.
Set the child's position before calling render().
Nested containers use their own x/y during render() to position descendants. Rendering first and assigning child.x / child.y afterwards leaves grandchildren with stale coordinates, so nested mouse hit detection drifts.
Suggested fix
row {
children.forEach { child ->
- val renderedChild = child.render(childAvailableWidth, resolvedHeight)
- val lines = renderedChild.lines()
-
child.x = currentX
child.y = this@Inline.y + this@Inline.padding + (if (this@Inline.borderStyle != BorderStyle.NONE) 1 else 0)
+ val renderedChild = child.render(childAvailableWidth, resolvedHeight)
+ val lines = renderedChild.lines()
child.widthPx = lines.maxOfOrNull { visibleLength(it) } ?: 0
child.heightPx = lines.size
currentX += child.widthPx
cell(Text(renderedChild))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Inline.kt`
around lines 42 - 52, In the Inline rendering loop (the children.forEach block
in Inline.kt) set the child's position (child.x and child.y) before invoking
child.render(...) so nested containers compute correct coordinates for their
descendants; move the assignment of child.x = currentX and child.y =
this@Inline.y + padding + border offset to precede the call to
child.render(childAvailableWidth, resolvedHeight), then compute
child.widthPx/heightPx from the rendered output and advance currentX and emit
the cell(Text(renderedChild)) as before.
Comment on lines
+60
to
+77
| override fun onEvent(event: BobaEvent): Boolean { | ||
| when (event) { | ||
| is BobaEvent.Key -> { | ||
| when { | ||
| KeyCodes.isUp(event.code) -> { | ||
| currentIndex = (currentIndex - 1 + options.size) % options.size | ||
| return true | ||
| } | ||
| KeyCodes.isDown(event.code) -> { | ||
| currentIndex = (currentIndex + 1) % options.size | ||
| return true | ||
| } | ||
| is BobaEvent.Mouse -> { | ||
| if (event.action == MouseAction.PRESS) { | ||
| val clickedIndex = event.y - startLine | ||
| if (clickedIndex in options.indices) { | ||
| currentIndex = clickedIndex | ||
| toggle(currentIndex) | ||
| printList() | ||
| } | ||
| } | ||
| event.code == SPACE.key -> { | ||
| toggle(currentIndex) | ||
| return true | ||
| } | ||
| event.code == ENTER.key || event.code == 'q'.code || event.code == 'Q'.code -> { | ||
| onComplete(selected) |
There was a problem hiding this comment.
Guard the empty-options case before modulo/toggle access.
With options = emptyList(), Up/Down evaluates % options.size and Space reaches toggle(0), so the first key press throws. Please either reject empty lists in the constructor or branch around the modulo/index access here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`
around lines 60 - 77, The onEvent handler in MultiSelectionList uses modulo
arithmetic and toggle/currentIndex access without guarding empty options, which
crashes when options.isEmpty(); fix by short-circuiting when options.isEmpty()
(e.g., at the top of onEvent return false or handle Enter/Q accordingly) or
enforce non-empty lists in the MultiSelectionList constructor (validate options
and throw/require). Specifically, modify MultiSelectionList.onEvent to check
options.isEmpty() before any use of currentIndex, options.size,
toggle(currentIndex) or reading selected, or add a constructor check for
options.isEmpty() and reject/throw to ensure callers cannot pass an empty list.
Comment on lines
+82
to
+89
| is BobaEvent.Mouse -> { | ||
| if (event.action == MouseAction.PRESS) { | ||
| val startLine = y + (if (borderStyle != BorderStyle.NONE) 1 else 0) + padding + 1 | ||
| val clickedIndex = event.y - startLine | ||
| if (clickedIndex in options.indices) { | ||
| currentIndex = clickedIndex | ||
| toggle(currentIndex) | ||
| return true |
There was a problem hiding this comment.
Mouse selection still assumes fixed row geometry.
The click math uses a hard-coded start line and one row per option, and it never checks event.x. Wrapped prompts/options or clicks on the same Y row outside the component will toggle the wrong item.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`
around lines 82 - 89, The mouse handler in MultiSelectionList (inside the
BobaEvent.Mouse branch) assumes one row per option and ignores event.x, causing
incorrect toggles for wrapped options or clicks outside the component; update
the handler to compute the component's top Y (use the same borderStyle/padding
logic already used for rendering), compute each option's rendered height
(accounting for wrapping/line breaks and any per-option rowHeight), build a
mapping from screen Y ranges to option indices, and only toggle when event.x
falls within the component's left/right bounds and event.y maps to a specific
option; modify the BobaEvent.Mouse logic that sets
startLine/clickedIndex/currentIndex and calls toggle(currentIndex) to use this
bounds-checked, wrap-aware mapping instead.
Comment on lines
+79
to
+88
| Screen.SINGLE_SELECT -> Stack(listOf( | ||
| SelectionList("Single Selection Demo:", listOf("Option A", "Option B", "Option C"), | ||
| onSelect = { App.lastSelection = it; App.currentScreen = Screen.MAIN }), | ||
| backButton | ||
| )) | ||
| Screen.MULTI_SELECT -> Stack(listOf( | ||
| MultiSelectionList("Multi Selection Demo:", listOf("Red", "Green", "Blue", "Cyan", "Yellow"), | ||
| onComplete = { App.lastMultiSelection = it; App.currentScreen = Screen.MAIN }), | ||
| backButton | ||
| )) |
There was a problem hiding this comment.
Single- and multi-select demos never receive input.
These lists are rebuilt inside render(), then Lines 106-107 forward events only to backButton. That makes both screens non-interactive today, and even if you wire them later, recreating the components here will reset their cursor/selection state every frame.
💡 Suggested shape
+ private val singleSelectDemo = SelectionList(
+ "Single Selection Demo:",
+ listOf("Option A", "Option B", "Option C"),
+ onSelect = { App.lastSelection = it; App.currentScreen = Screen.MAIN }
+ )
+ private val multiSelectDemo = MultiSelectionList(
+ "Multi Selection Demo:",
+ listOf("Red", "Green", "Blue", "Cyan", "Yellow"),
+ onComplete = { App.lastMultiSelection = it; App.currentScreen = Screen.MAIN }
+ )
...
- Screen.SINGLE_SELECT -> Stack(listOf(
- SelectionList("Single Selection Demo:", listOf("Option A", "Option B", "Option C"),
- onSelect = { App.lastSelection = it; App.currentScreen = Screen.MAIN }),
- backButton
- ))
- Screen.MULTI_SELECT -> Stack(listOf(
- MultiSelectionList("Multi Selection Demo:", listOf("Red", "Green", "Blue", "Cyan", "Yellow"),
- onComplete = { App.lastMultiSelection = it; App.currentScreen = Screen.MAIN }),
- backButton
- ))
+ Screen.SINGLE_SELECT -> Stack(listOf(singleSelectDemo, backButton))
+ Screen.MULTI_SELECT -> Stack(listOf(multiSelectDemo, backButton))
...
- Screen.SINGLE_SELECT -> backButton.onEvent(event)
- Screen.MULTI_SELECT -> backButton.onEvent(event)
+ Screen.SINGLE_SELECT -> singleSelectDemo.onEvent(event) || backButton.onEvent(event)
+ Screen.MULTI_SELECT -> multiSelectDemo.onEvent(event) || backButton.onEvent(event)Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample/src/main/kotlin/com/example/App.kt` around lines 79 - 88, The
SelectionList and MultiSelectionList are recreated every render inside render(),
and events are only forwarded to backButton so the lists never receive input and
their cursor state resets; fix by making the SelectionList and
MultiSelectionList persistent (create and store instances outside render or keep
them in App state) instead of rebuilding them each frame and update render() to
forward input/events to those stored instances as well as to backButton (ensure
Screen.SINGLE_SELECT and Screen.MULTI_SELECT branches reference the persisted
SelectionList/MultiSelectionList objects and not freshly constructed ones).
| Screen.NESTED -> backButton.onEvent(event) | ||
| Screen.MORDANT -> backButton.onEvent(event) | ||
| Screen.EXPANDABLE -> expandableLayout.onEvent(event) || backButton.onEvent(event) | ||
| Screen.COORDINATES -> coordinatesDemo.onEvent(event) || backButton.onEvent(event) |
There was a problem hiding this comment.
Don't consume every key in CoordinatesDemo.
Line 191 returns true for all key events, even when no arrow key matched. Because Line 105 uses coordinatesDemo.onEvent(event) || backButton.onEvent(event), q never reaches the back button on this screen.
💡 Minimal fix
override fun onEvent(event: BobaEvent): Boolean {
- if (event is BobaEvent.Key) {
- when {
- KeyCodes.isDown(event.code) -> yPos++
- KeyCodes.isUp(event.code) -> yPos--
- KeyCodes.isLeft(event.code) -> xPos--
- KeyCodes.isRight(event.code) -> xPos++
- }
- return true
- }
- return false
+ if (event !is BobaEvent.Key) return false
+ return when {
+ KeyCodes.isDown(event.code) -> { yPos++; true }
+ KeyCodes.isUp(event.code) -> { yPos--; true }
+ KeyCodes.isLeft(event.code) -> { xPos--; true }
+ KeyCodes.isRight(event.code) -> { xPos++; true }
+ else -> false
+ }
}Also applies to: 183-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample/src/main/kotlin/com/example/App.kt` at line 105, CoordinatesDemo
currently consumes every key event: change the CoordinatesDemo.onEvent(event)
implementation so it returns true only when it actually handles an input (e.g.,
an arrow key moved the cursor) and returns false for unhandled keys; this
ensures the Screen.COORDINATES branch expression (coordinatesDemo.onEvent(event)
|| backButton.onEvent(event)) allows backButton.onEvent(event) to run for keys
like 'q'. Update the onEvent method in CoordinatesDemo (and the same logic in
the block covering lines ~183–191) to detect handled keys explicitly and return
false otherwise.
| WasmScreen.MENU -> mainMenu | ||
| WasmScreen.LAYOUT -> Stack(listOf(buildWasmLayout(), backButton)) | ||
| WasmScreen.MORDANT -> Stack(listOf(buildWasmMordant(), backButton)) | ||
| WasmScreen.EXPANDABLE -> Stack(listOf(buildWasmExpandable(), backButton)) |
There was a problem hiding this comment.
Reuse one ExpandableComponent instance for the screen.
Both branches call buildWasmExpandable() anew. The event path then targets a fresh component that has never been rendered, so its geometry/state is discarded immediately and the demo cannot stay expanded.
Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/wasmJsMain/kotlin/Main.kt` at line 66, WasmScreen.EXPANDABLE
currently constructs a new ExpandableComponent each render by calling
buildWasmExpandable() twice; create and reuse a single ExpandableComponent
instance instead: instantiate buildWasmExpandable() once (e.g., val expandable =
buildWasmExpandable()) and pass that same expandable into the Stack and any
event handlers rather than calling buildWasmExpandable() again so the
ExpandableComponent instance retains its geometry/state across renders; update
references in the WasmScreen.EXPANDABLE branch (and the corresponding similar
branch at lines ~99) to use the shared expandable variable and remove duplicate
calls.
Comment on lines
+68
to
+75
| WasmScreen.SELECTION -> Stack(listOf( | ||
| SelectionList("What's your favourite Boba?", listOf("Milk Tea", "Matcha", "Taro", "Brown Sugar"), onSelect = { currentWasmScreen = WasmScreen.MENU }), | ||
| backButton | ||
| )) | ||
| WasmScreen.MULTI -> Stack(listOf( | ||
| MultiSelectionList("Toppings:", listOf("Pearls", "Grass Jelly", "Pudding", "Aloe Vera", "Red Bean"), onComplete = { currentWasmScreen = WasmScreen.MENU }), | ||
| backButton | ||
| )) |
There was a problem hiding this comment.
The selection demo screens never receive their own input.
Each render builds a brand-new SelectionList / MultiSelectionList, and onEvent() only forwards to backButton. That resets list state every frame and makes the screens impossible to navigate or complete. These components need a stable instance across frames, or at least recreation on screen entry rather than on every render.
Also applies to: 101-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/wasmJsMain/kotlin/Main.kt` around lines 68 - 75, The SelectionList
and MultiSelectionList are being recreated every render in the
WasmScreen.SELECTION and WasmScreen.MULTI branches which resets their internal
state and prevents navigation; instead create and retain stable instances (or
initialize them only when the screen is entered) and reuse them across frames:
move creation of SelectionList and MultiSelectionList out of the per-frame
render branches into persistent properties (or lazily initialize when
currentWasmScreen transitions to WasmScreen.SELECTION / WasmScreen.MULTI), keep
the existing onSelect/onComplete callbacks to set currentWasmScreen, and
continue forwarding events to backButton as before.
Comment on lines
+150
to
+158
| override fun onEvent(event: BobaEvent): Boolean { | ||
| if (event is BobaEvent.Key) { | ||
| when { | ||
| KeyCodes.isDown(event.code) -> if (y < 7) render(x, ++y) | ||
| KeyCodes.isUp(event.code) -> if (y > 0) render(x, --y) | ||
| KeyCodes.isLeft(event.code) -> if (x > 0) render(--x, y) | ||
| KeyCodes.isRight(event.code) -> if (x < 7) render(++x, y) | ||
| event.code == KeyCodes.ENTER.key || event.code == KeyCodes.SPACE.key -> { | ||
| terminal.clear() | ||
| terminal.write("${color("Step 3: Custom Navigation", YELLOW)}\n") | ||
| terminal.write("Confirmed position: $x, $y\n") | ||
| return | ||
| } | ||
| KeyCodes.isDown(event.code) -> if (yPos < 7) yPos++ | ||
| KeyCodes.isUp(event.code) -> if (yPos > 0) yPos-- | ||
| KeyCodes.isLeft(event.code) -> if (xPos > 0) xPos-- | ||
| KeyCodes.isRight(event.code) -> if (xPos < 7) xPos++ | ||
| } | ||
| return true |
There was a problem hiding this comment.
Only consume keys that actually move the cursor.
The unconditional return true swallows every key, including q, so backButton.onEvent(event) never runs on the coordinates screen.
Suggested fix
override fun onEvent(event: BobaEvent): Boolean {
if (event is BobaEvent.Key) {
- when {
- KeyCodes.isDown(event.code) -> if (yPos < 7) yPos++
- KeyCodes.isUp(event.code) -> if (yPos > 0) yPos--
- KeyCodes.isLeft(event.code) -> if (xPos > 0) xPos--
- KeyCodes.isRight(event.code) -> if (xPos < 7) xPos++
- }
- return true
+ return when {
+ KeyCodes.isDown(event.code) -> { if (yPos < 7) yPos++; true }
+ KeyCodes.isUp(event.code) -> { if (yPos > 0) yPos--; true }
+ KeyCodes.isLeft(event.code) -> { if (xPos > 0) xPos--; true }
+ KeyCodes.isRight(event.code) -> { if (xPos < 7) xPos++; true }
+ else -> false
+ }
}
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/wasmJsMain/kotlin/Main.kt` around lines 150 - 158, In onEvent(event:
BobaEvent): Boolean (Main.kt), only consume the key event when it actually moves
the cursor: compute a local flag (e.g., moved = false) and set it true inside
the KeyCodes.isDown/isUp/isLeft/isRight branches only when xPos or yPos is
changed (for example, when yPos < 7 then yPos++; moved = true). After the when
block return moved instead of unconditionally returning true so non-movement
keys (like 'q') propagate to backButton.onEvent(event).
… fix mouse events - Introduce central `Boba.run` loop for unified rendering and event handling - Refactor `BobaComponent` with `onEvent` and `tick` lifecycle methods - Update layout components to propagate coordinates and events to children - Port interactive components to the new declarative model - Spruce up `ConsoleColors` with Bubble Tea-inspired RGB palette - Migrate sample and WASM apps to the new API - Fix mouse tracking by enabling it in the core loop and correcting coordinate offsets - Fix Java 11 badge sizing on the landing page Co-authored-by: dylanwatsonsoftware <2309644+dylanwatsonsoftware@users.noreply.github.com>
… fix UI/UX issues - Introduce central `Boba.run` loop for unified rendering and event handling - Refactor `BobaComponent` with `onEvent` and `tick` lifecycle methods - Add `getContentStartX()` and `getContentStartY()` for centralized coordinate logic - Update layout components to propagate coordinates and events to children - Port interactive components to the new declarative model and fix hit detection - Spruce up `ConsoleColors` with Bubble Tea-inspired RGB palette - Migrate sample and WASM apps to the new API - Fix Java 11 badge sizing and mobile responsiveness on the landing page Co-authored-by: dylanwatsonsoftware <2309644+dylanwatsonsoftware@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.kt (2)
46-60:⚠️ Potential issue | 🟠 MajorGuard empty
optionsbefore navigating or confirming.Up/Down still use
% options.size, and confirm still readsoptions[currentIndex], so the first key event throws when the list is empty. Reject empty lists up front or short-circuit these paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.kt` around lines 46 - 60, The onEvent handler doesn't guard against empty options causing modulo-by-zero or index-out-of-bounds when handling Key events; update onEvent (in SelectionList) to first check options.isEmpty() and return false (or no-op) for navigation/confirmation keys, or alternatively clamp currentIndex and skip onSelect when options.isEmpty(); ensure any use of currentIndex, options.size and options[currentIndex] (inside onEvent and where ENTER/SPACE/'q'/'Q' are handled) is protected by that guard so no division by zero or invalid indexing occurs.
65-76:⚠️ Potential issue | 🟠 MajorMouse selection still uses fixed row math.
clickedIndex = event.y - startLineassumes the question and every option occupy exactly one visual row, and it never checksevent.x. Once the question/options wrap, clicks outside the component or on wrapped rows can select the wrong item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.kt` around lines 65 - 76, The mouse handling in the BobaEvent.Mouse/MouseAction.PRESS branch incorrectly computes clickedIndex using a fixed row subtraction (clickedIndex = event.y - startLine), which breaks when the question or options wrap and ignores event.x; fix it by mapping the mouse y to the actual visual line of the component using the layout/line metrics rather than assuming one row per item: compute the number of visual lines occupied by getContentStartY() (question) and for each option in options calculate its wrapped line count (using the same width/wrapping logic as rendering) to determine the cumulative start/end line ranges, then check event.x is inside the component bounds and find which option range contains event.y to set currentIndex or call onSelect(selected); update the logic in the block referencing clickedIndex/currentIndex/onSelect/getContentStartY() accordingly so wrapped rows and out-of-bounds clicks are handled correctly.bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt (2)
55-59:⚠️ Potential issue | 🟠 MajorDon't consume SPACE/ENTER unless this component is actually targeted.
This still flips
expandedon every SPACE/ENTER, so the firstExpandableComponentthat sees the event can preempt later interactive siblings. Gate the toggle behind focus/targeted routing and returnfalseotherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt` around lines 55 - 59, The key handler in ExpandableComponent currently toggles expanded for any BobaEvent.Key with SPACE/ENTER regardless of focus; change the logic in the BobaEvent.Key branch to first check whether this component is the focused/targeted receiver (use whatever focus/targeted flag or function the UI framework provides — e.g. isFocused, isTargeted, or a hit-test/target check) and only flip expanded = !expanded and return true when that check passes; if the component is not targeted, do not consume the event and return false so later siblings can handle SPACE/ENTER.
44-47:⚠️ Potential issue | 🟠 MajorHeader hit-testing still assumes a single unwrapped row.
render()wraps the component first, butcurrentlyHoveredstill usesevent.y == titleLineand a flat character count. Once the header wraps inside a narrow layout, only the first visual row remains hoverable/clickable.Also applies to: 52-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt` around lines 44 - 47, Header hit-testing in currentlyHovered incorrectly assumes the title is a single unwrapped row; compute the wrapped title box the same way render() does (use wrapInBox(title.toString().trimEnd('\n'), availableWidth, availableHeight) or reuse the wrapped result variable) and determine the header's visual row range and per-line visible lengths with visibleLength so you check event.y against the title's wrapped row indices and event.x against the corresponding wrapped line's visible length; update the logic in currentlyHovered (and the similar logic around lines 52-65) to use that wrapped-title row range and per-line widths instead of titleLine and a flat character count.bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt (2)
64-74:⚠️ Potential issue | 🟠 MajorGuard empty
optionsbefore navigating or toggling.Up/Down still do
% options.size, and SPACE still reachestoggle(currentIndex)with no empty-list guard.options = emptyList()will throw as soon as the user navigates or toggles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt` around lines 64 - 74, Guard against options.isEmpty() in the key handling branches so navigation and toggling never perform modulo by zero or call toggle on an empty list: in the KeyCodes.isUp and KeyCodes.isDown branches, check if options.isEmpty() and if so skip updating currentIndex and return false (or otherwise consume appropriately), otherwise compute currentIndex = (currentIndex +/- 1 + options.size) % options.size; also in the event.code == SPACE.key branch, check options.isEmpty() before calling toggle(currentIndex) and skip/return false when empty. Ensure you reference and modify the existing currentIndex, options, toggle(...) logic inside MultiSelectionList's key handler.
82-89:⚠️ Potential issue | 🟠 MajorMouse toggling still uses fixed row math.
clickedIndex = event.y - startLineassumes one visual row per option and never checksevent.x. Wrapped question/item text or clicks outside the component can toggle the wrong entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt` around lines 82 - 89, The mouse handler in BobaEvent.Mouse (MouseAction.PRESS) currently computes clickedIndex as event.y - getContentStartY() which assumes one row per option and ignores event.x; change the hit detection to (1) verify event.x is within the component/content width before accepting a click, and (2) map the y coordinate to the correct option by iterating the rendered layout (use the same logic that performs word-wrapping/line measurement for options—e.g., compute per-option rendered heights or row offsets used by getContentStartY()) to find the option whose vertical span contains event.y, then set currentIndex and call toggle(currentIndex) only when that mapping succeeds. Ensure you update the code around currentIndex, toggle(), getContentStartY(), and the BobaEvent.Mouse branch to use this mapping instead of the fixed clickedIndex calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.kt`:
- Around line 21-25: The hitbox is hard-coded to one row and 16 cells; change
onEvent to use the measured bounds produced by render/wrapInBox instead of fixed
values: use the computed widthPx and heightPx (and any visibleLength logic)
returned after calling wrapInBox(styled, availableWidth, availableHeight) to
determine the button rectangle and test the incoming click/touch coordinates
against that rectangle; update both occurrences (the onEvent handling around the
first render and the second occurrence at lines 33-37) so they check x in [x0,
x0+widthPx) and y in [y0, y0+heightPx) rather than assuming height=1 and
width=16. Ensure you reference render(), wrapInBox(...), onEvent, widthPx and
heightPx when making the change.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.kt`:
- Around line 23-33: The idle loop currently uses terminal.readEvent(50) plus
delay(10) which forces no-input frames to ~60ms; instead consolidate to a single
per-frame budget: introduce a targetFrameMs (e.g. 16L) and measure elapsed time
at frame start, call terminal.readEvent(remainingTimeout) where remainingTimeout
= max(0, targetFrameMs - elapsedSoFar), then call root.onEvent(event) and
root.tick(now - lastTick) and only call delay(remainingDelay) if you still have
positive remaining time after event polling and ticking; update uses of
terminal.readEvent, root.tick, lastTick and delay to implement this single-frame
budget behavior.
- Line 10: The frame-timing uses wall-clock time (lastTick =
currentTimeMillis()) which can jump; update both platform-specific
implementations in Boba.kt to use monotonic clocks: on JVM replace
System.currentTimeMillis()/currentTimeMillis() with System.nanoTime() and on
WASM replace Date.now() with performance.now(); keep lastTick and delta
calculations consistent by storing the same unit on both sides (e.g., use
nanoseconds everywhere or convert nano→ms when computing delta) and update any
delta math that uses lastTick so it no longer produces negative/large values
after clock changes.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`:
- Around line 76-77: The onComplete callback is being passed the live MutableSet
"selected" which can be mutated after completion; change the call in the
key-handling branch (the block that checks event.code == ENTER.key || event.code
== 'q'.code || event.code == 'Q'.code) to pass a snapshot instead by invoking
onComplete(selected.toSet()) so the caller receives an immutable copy rather
than the component's live MutableSet.
---
Duplicate comments:
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.kt`:
- Around line 55-59: The key handler in ExpandableComponent currently toggles
expanded for any BobaEvent.Key with SPACE/ENTER regardless of focus; change the
logic in the BobaEvent.Key branch to first check whether this component is the
focused/targeted receiver (use whatever focus/targeted flag or function the UI
framework provides — e.g. isFocused, isTargeted, or a hit-test/target check) and
only flip expanded = !expanded and return true when that check passes; if the
component is not targeted, do not consume the event and return false so later
siblings can handle SPACE/ENTER.
- Around line 44-47: Header hit-testing in currentlyHovered incorrectly assumes
the title is a single unwrapped row; compute the wrapped title box the same way
render() does (use wrapInBox(title.toString().trimEnd('\n'), availableWidth,
availableHeight) or reuse the wrapped result variable) and determine the
header's visual row range and per-line visible lengths with visibleLength so you
check event.y against the title's wrapped row indices and event.x against the
corresponding wrapped line's visible length; update the logic in
currentlyHovered (and the similar logic around lines 52-65) to use that
wrapped-title row range and per-line widths instead of titleLine and a flat
character count.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`:
- Around line 64-74: Guard against options.isEmpty() in the key handling
branches so navigation and toggling never perform modulo by zero or call toggle
on an empty list: in the KeyCodes.isUp and KeyCodes.isDown branches, check if
options.isEmpty() and if so skip updating currentIndex and return false (or
otherwise consume appropriately), otherwise compute currentIndex = (currentIndex
+/- 1 + options.size) % options.size; also in the event.code == SPACE.key
branch, check options.isEmpty() before calling toggle(currentIndex) and
skip/return false when empty. Ensure you reference and modify the existing
currentIndex, options, toggle(...) logic inside MultiSelectionList's key
handler.
- Around line 82-89: The mouse handler in BobaEvent.Mouse (MouseAction.PRESS)
currently computes clickedIndex as event.y - getContentStartY() which assumes
one row per option and ignores event.x; change the hit detection to (1) verify
event.x is within the component/content width before accepting a click, and (2)
map the y coordinate to the correct option by iterating the rendered layout (use
the same logic that performs word-wrapping/line measurement for options—e.g.,
compute per-option rendered heights or row offsets used by getContentStartY())
to find the option whose vertical span contains event.y, then set currentIndex
and call toggle(currentIndex) only when that mapping succeeds. Ensure you update
the code around currentIndex, toggle(), getContentStartY(), and the
BobaEvent.Mouse branch to use this mapping instead of the fixed clickedIndex
calculation.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.kt`:
- Around line 46-60: The onEvent handler doesn't guard against empty options
causing modulo-by-zero or index-out-of-bounds when handling Key events; update
onEvent (in SelectionList) to first check options.isEmpty() and return false (or
no-op) for navigation/confirmation keys, or alternatively clamp currentIndex and
skip onSelect when options.isEmpty(); ensure any use of currentIndex,
options.size and options[currentIndex] (inside onEvent and where
ENTER/SPACE/'q'/'Q' are handled) is protected by that guard so no division by
zero or invalid indexing occurs.
- Around line 65-76: The mouse handling in the BobaEvent.Mouse/MouseAction.PRESS
branch incorrectly computes clickedIndex using a fixed row subtraction
(clickedIndex = event.y - startLine), which breaks when the question or options
wrap and ignores event.x; fix it by mapping the mouse y to the actual visual
line of the component using the layout/line metrics rather than assuming one row
per item: compute the number of visual lines occupied by getContentStartY()
(question) and for each option in options calculate its wrapped line count
(using the same width/wrapping logic as rendering) to determine the cumulative
start/end line ranges, then check event.x is inside the component bounds and
find which option range contains event.y to set currentIndex or call
onSelect(selected); update the logic in the block referencing
clickedIndex/currentIndex/onSelect/getContentStartY() accordingly so wrapped
rows and out-of-bounds clicks are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa8145a5-c5e5-4a8e-bd04-eaf793536e60
📒 Files selected for processing (7)
bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaComponent.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/ExpandableComponent.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.ktbobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/SelectionList.ktweb/src/wasmJsMain/resources/index.html
✅ Files skipped from review due to trivial changes (1)
- web/src/wasmJsMain/resources/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BobaComponent.kt
Comment on lines
+21
to
+25
| val result = wrapInBox(styled, availableWidth, availableHeight) | ||
| val lines = result.lines() | ||
| widthPx = lines.maxOfOrNull { visibleLength(it) } ?: 0 | ||
| heightPx = lines.size | ||
| return result |
There was a problem hiding this comment.
The back-button hitbox is still a fixed one-row, 16-cell box.
render() goes through wrapInBox(...), but onEvent assumes the label always occupies one row and exactly 16 cells. That can miss clicks on narrow layouts and on terminals where ← renders wider than one cell. Reuse the label's measured bounds instead of hard-coding them.
Also applies to: 33-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/BackButton.kt`
around lines 21 - 25, The hitbox is hard-coded to one row and 16 cells; change
onEvent to use the measured bounds produced by render/wrapInBox instead of fixed
values: use the computed widthPx and heightPx (and any visibleLength logic)
returned after calling wrapInBox(styled, availableWidth, availableHeight) to
determine the button rectangle and test the incoming click/touch coordinates
against that rectangle; update both occurrences (the onEvent handling around the
first render and the second occurrence at lines 33-37) so they check x in [x0,
x0+widthPx) and y in [y0, y0+heightPx) rather than assuming height=1 and
width=16. Ensure you reference render(), wrapInBox(...), onEvent, widthPx and
heightPx when making the change.
| suspend fun run(terminal: Terminal, root: BobaComponent) { | ||
| terminal.clear() | ||
| terminal.enableMouseTracking(true) | ||
| var lastTick = currentTimeMillis() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'Boba\.(jvm|wasm)\.kt$' bobatea/src | while read -r f; do
echo "== $f =="
sed -n '1,120p' "$f"
done
rg -n 'currentTimeMillis\(' bobatea/srcRepository: dylanwatsonsoftware/bobatea
Length of output: 1544
Use monotonic time for frame-delta calculations instead of wall-clock time.
Both actual implementations use wall-clock APIs—JVM uses System.currentTimeMillis() and WASM uses Date.now()—which can jump backward after system clock adjustments. This causes frame deltas to become negative or unexpectedly large. Use System.nanoTime() on JVM and performance.now() on WASM instead.
Affected: lines 10, 28-30, 42 in Boba.kt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.kt`
at line 10, The frame-timing uses wall-clock time (lastTick =
currentTimeMillis()) which can jump; update both platform-specific
implementations in Boba.kt to use monotonic clocks: on JVM replace
System.currentTimeMillis()/currentTimeMillis() with System.nanoTime() and on
WASM replace Date.now() with performance.now(); keep lastTick and delta
calculations consistent by storing the same unit on both sides (e.g., use
nanoseconds everywhere or convert nano→ms when computing delta) and update any
delta math that uses lastTick so it no longer produces negative/large values
after clock changes.
Comment on lines
+23
to
+33
| val event = terminal.readEvent(50) | ||
| if (event != null) { | ||
| root.onEvent(event) | ||
| } | ||
|
|
||
| val now = currentTimeMillis() | ||
| root.tick(now - lastTick) | ||
| lastTick = now | ||
|
|
||
| // Yield to other coroutines | ||
| delay(10) |
There was a problem hiding this comment.
The idle loop is capped at roughly 16 FPS.
readEvent(50) and then delay(10) make no-input frames take at least ~60 ms, so tick-driven animations will only update around 16 times per second. Fold both waits into a single frame budget, or make the extra delay conditional on how long event polling already blocked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/Boba.kt`
around lines 23 - 33, The idle loop currently uses terminal.readEvent(50) plus
delay(10) which forces no-input frames to ~60ms; instead consolidate to a single
per-frame budget: introduce a targetFrameMs (e.g. 16L) and measure elapsed time
at frame start, call terminal.readEvent(remainingTimeout) where remainingTimeout
= max(0, targetFrameMs - elapsedSoFar), then call root.onEvent(event) and
root.tick(now - lastTick) and only call delay(remainingDelay) if you still have
positive remaining time after event polling and ticking; update uses of
terminal.readEvent, root.tick, lastTick and delay to implement this single-frame
budget behavior.
Comment on lines
+76
to
+77
| event.code == ENTER.key || event.code == 'q'.code || event.code == 'Q'.code -> { | ||
| onComplete(selected) |
There was a problem hiding this comment.
Pass a snapshot to onComplete.
selected is the component's live MutableSet. Handing that same instance to the callback lets later toggles mutate the caller's "completed" result. Send selected.toSet() instead.
Suggested change
- onComplete(selected)
+ onComplete(selected.toSet())📝 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.
Suggested change
| event.code == ENTER.key || event.code == 'q'.code || event.code == 'Q'.code -> { | |
| onComplete(selected) | |
| event.code == ENTER.key || event.code == 'q'.code || event.code == 'Q'.code -> { | |
| onComplete(selected.toSet()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bobatea/src/commonMain/kotlin/com/github/dylanwatsonsoftware/bobatea/MultiSelectionList.kt`
around lines 76 - 77, The onComplete callback is being passed the live
MutableSet "selected" which can be mutated after completion; change the call in
the key-handling branch (the block that checks event.code == ENTER.key ||
event.code == 'q'.code || event.code == 'Q'.code) to pass a snapshot instead by
invoking onComplete(selected.toSet()) so the caller receives an immutable copy
rather than the component's live MutableSet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This major refactor transforms BobaTea into a modern, declarative TUI framework inspired by Go's Bubble Tea.
Key changes:
onEvent(BobaEvent): Booleanfor state updates andtick(deltaMs: Long)for animations, replacing imperative interaction loops.Boba.runloop incommonMainmanages the rendering, event dispatching, and timing, simplifying application development.x,y) and size (widthPx,heightPx), allowing layout components likeStackandInlineto support reliable mouse hit detection for nested elements.ConsoleColorsusing RGB support, bringing the visual style closer to the original Bubble Tea framework.Terminalimplementations to support timed event reading, ensuring smooth frame rates and responsive UIs.The sample applications and WASM demo have been completely rewritten to showcase these new patterns.
PR created automatically by Jules for task 17122179855656149949 started by @dylanwatsonsoftware
Summary by CodeRabbit
New Features
Improvements
Style