feat(ui): smooth auto-scroll and task progress preview - #5
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a progress tracking and polling mechanism for subagent sessions, exposing a new get_progress Lua API and updating the task plugin to display live previews of running tools. It also refactors code view rendering to support custom themes, pauses auto-scrolling on user interaction in the UI, and cleans up minor code patterns. The review feedback highlights two key issues: a potential busy-looping bug in get_progress due to accumulated channel notifications, and a potential deadlock in the task plugin if do_prompt fails before closing the session.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async fn get_progress(lua: Lua, this: mlua::UserDataRef<LuaSession>) -> LuaResult<Pair<Table>> { | ||
| let progress = Arc::clone(&this.progress); | ||
| let notify = pin!(progress.rx.recv_async()); | ||
| let timeout = pin!(smol::Timer::after(Duration::from_millis( | ||
| PROGRESS_TIMEOUT_MS | ||
| ))); | ||
| let _ = select(notify, timeout).await; |
There was a problem hiding this comment.
When multiple tool events (like ToolStart and ToolDone) occur in rapid succession, multiple notifications are sent to the unbounded channel progress.rx. If get_progress is called after these events have accumulated, it will immediately return multiple times without waiting for new progress or respecting the PROGRESS_TIMEOUT_MS interval. This can lead to redundant redraws and busy-looping on the Lua side.
Draining the channel before waiting ensures that we only block on new progress notifications or the timeout.
| async fn get_progress(lua: Lua, this: mlua::UserDataRef<LuaSession>) -> LuaResult<Pair<Table>> { | |
| let progress = Arc::clone(&this.progress); | |
| let notify = pin!(progress.rx.recv_async()); | |
| let timeout = pin!(smol::Timer::after(Duration::from_millis( | |
| PROGRESS_TIMEOUT_MS | |
| ))); | |
| let _ = select(notify, timeout).await; | |
| async fn get_progress(lua: Lua, this: mlua::UserDataRef<LuaSession>) -> LuaResult<Pair<Table>> { | |
| let progress = Arc::clone(&this.progress); | |
| while progress.rx.try_recv().is_ok() {} | |
| let notify = pin!(progress.rx.recv_async()); | |
| let timeout = pin!(smol::Timer::after(Duration::from_millis( | |
| PROGRESS_TIMEOUT_MS | |
| ))); | |
| let _ = select(notify, timeout).await; |
| local function do_prompt() | ||
| local message = input.prompt | ||
| if validator then | ||
| message = message .. STRUCTURED_OUTPUT_PROMPT_SUFFIX | ||
| end | ||
| local result, err = sess:prompt(message) | ||
| local retries = 0 | ||
| while not err and validator and not captured and retries < MAX_STRUCTURED_RETRIES do | ||
| retries = retries + 1 | ||
| result, err = sess:prompt(NUDGE_MISSING) | ||
| end | ||
| if err then | ||
| return { llm_output = "sub-agent error: " .. err, is_error = true } | ||
| end | ||
| if validator and not captured then | ||
| local msg = last_errors and (STRUCTURED_INVALID_ERROR .. ":\n" .. last_errors) or STRUCTURED_MISSING_ERROR | ||
| return { llm_output = msg, is_error = true } | ||
| end | ||
| return { llm_output = captured and maki.json.encode(captured) or result.text, format = "markdown" } | ||
| end |
There was a problem hiding this comment.
If do_prompt encounters an error or throws before completing, sess:close() (which triggers progress.set_done()) will never be called. Since do_poll is a blocking loop that only terminates when progress.done is true, this can cause maki.async.gather to hang indefinitely because do_poll keeps sess alive (preventing garbage collection and the Drop implementation from running).
To prevent this potential deadlock, we should ensure sess:close() is called even if do_prompt fails.
local function do_prompt()
local ok, res = pcall(function()
local message = input.prompt
if validator then
message = message .. STRUCTURED_OUTPUT_PROMPT_SUFFIX
end
local result, err = sess:prompt(message)
local retries = 0
while not err and validator and not captured and retries < MAX_STRUCTURED_RETRIES do
retries = retries + 1
result, err = sess:prompt(NUDGE_MISSING)
end
if err then
return { llm_output = "sub-agent error: " .. err, is_error = true }
end
if validator and not captured then
local msg = last_errors and (STRUCTURED_INVALID_ERROR .. ":\n" .. last_errors) or STRUCTURED_MISSING_ERROR
return { llm_output = msg, is_error = true }
end
return { llm_output = captured and maki.json.encode(captured) or result.text, format = "markdown" }
end)
sess:close()
if not ok then
error(res, 0)
end
return res
end
Clicking to expand a truncated tool or collapsed thinking would leave auto-scroll enabled. If the viewport was already near the bottom, the next render would smooth-scroll toward the new maximum, making the expanded content appear to jump away from the clicked row instead of expanding in place. Pause auto-scroll on user-initiated expansion toggles so the viewport stays anchored where the user clicked. Add a regression test that a truncated tool expansion does not trigger auto-scroll and does not move scroll_top. Signed-off-by: w0wl0lxd <w0wl0lxd@tuta.com>
💡 Codex Reviewhttps://github.com/w0wl0lxd/noon/blob/46dd01e414d360a458a9df42459eeb4f9b924173/plugins/task/init.lua#L266 Returning nil here after only scheduling ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…heme changes code_view diff tests compared rendered output to a hand-computed ground truth. Both paths read maki_highlight's global theme, but they captured it at different moments. Under cargo test another test (e.g. theme_picker) could call theme::set between the two snapshots, causing the assertion to fail. Thread a single maki_highlight theme Arc through render_diff and the test helper fg_in_context so the color comparison uses the exact same palette. Also expose Highlighter::new and Theme from maki_highlight so callers can construct a highlighter with an explicit theme instead of the global default. Signed-off-by: w0wl0lxd <w0wl0lxd@tuta.com>
…body Adds a live, expandable 5-line preview to task tool calls showing the subagent's current tool, elapsed timer, and recently finished tools. Also makes todo_write render its list as a clickable body in the chat, so users can expand it without relying on the Ctrl+T panel. Key changes: - maki.agent.Session gains :get_progress(), polled by the task plugin. - ProgressState tracks current tool, recent completions, and timer. - task handler runs the subagent prompt and a progress poller concurrently via maki.async.gather, streaming updates into a live buf via ToolView. - todo_write returns the todo list as a ToolView body on write/restore. - Drive-by clippy fixes in subcmd. Signed-off-by: w0wl0lxd <w0wl0lxd@tuta.com>
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds subagent progress tracking and Lua polling, integrates live progress into task previews, shares themes across diff highlighting, pauses message auto-scrolling during interactions, improves todo previews, and applies small behavior-preserving cleanups. ChangesAgent progress and UI behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskHandler
participant LuaSession
participant Progress
participant PreviewBuffer
TaskHandler->>LuaSession: start prompt and progress polling
LuaSession->>Progress: wait for tool updates or timeout
Progress-->>LuaSession: return progress snapshot
LuaSession-->>TaskHandler: provide progress data
TaskHandler->>PreviewBuffer: update live task preview
TaskHandler-->>TaskHandler: finish after prompt and polling complete
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Clicking a finished or in-progress tool output should freeze the viewport so the user can interact with or read the result. Previously only expansion toggles paused auto-scroll; snapshot/tool-request clicks did not, so a streaming follow-up could yank the viewport away. Add the pause to the snapshot click path and cover both running and done tool clicks with tests. Signed-off-by: w0wl0lxd <w0wl0lxd@tuta.com>
46dd01e to
3b06028
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
maki-lua/src/api/agent.rs (1)
813-861: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
progress.doneshould not be the signal for eachprompt()call
set_done()latchesdone = truefor the whole session, butdo_pollexits as soon as it sees that flag. Since the structured-output retry path callssess:prompt()multiple times on the same session, the preview stops updating after the first attempt and goes stale during later retries. Makedonemean session close only, or add a separate per-prompt completion signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@maki-lua/src/api/agent.rs` around lines 813 - 861, The prompt function’s progress completion call incorrectly marks the entire session done after each prompt, preventing do_poll from updating during retries. Remove or replace s.progress.set_done() in prompt so done remains reserved for session closure, while preserving any separate per-prompt completion signaling if the progress API provides one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@maki-lua/src/api/agent.rs`:
- Around line 708-718: Update add_recent and the surrounding process_tool_calls
progress state to track in-flight tool IDs or an active-tool count before
clearing current. Only clear current and allow the UI to fall back once all
parallel tools have completed, while preserving recent-tool recording and
completion counting for each ToolDone.
---
Outside diff comments:
In `@maki-lua/src/api/agent.rs`:
- Around line 813-861: The prompt function’s progress completion call
incorrectly marks the entire session done after each prompt, preventing do_poll
from updating during retries. Remove or replace s.progress.set_done() in prompt
so done remains reserved for session closure, while preserving any separate
per-prompt completion signaling if the progress API provides one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bd0aa82-a423-4191-b9f2-92c945917dbb
📒 Files selected for processing (10)
maki-highlight/src/lib.rsmaki-lua/src/api/agent.rsmaki-ui/src/components/code_view.rsmaki-ui/src/components/messages/mod.rsmaki-ui/src/components/messages/tests.rsmaki-ui/src/components/permission_prompt.rsplugins/task/init.luaplugins/todo_write/init.luasite/docs/content/lua-api/_index.mdsrc/cmd/subcmd.rs
📜 Review details
🧰 Additional context used
🪛 Luacheck (1.2.0)
plugins/task/init.lua
[warning] 90-90: shadowing upvalue 'description' on line 27
(W431)
[warning] 240-240: unused variable 'err'
(W211)
🔇 Additional comments (14)
maki-ui/src/components/permission_prompt.rs (1)
303-303: LGTM!src/cmd/subcmd.rs (3)
277-279: LGTM!
590-592: LGTM!
607-609: LGTM!maki-ui/src/components/messages/mod.rs (1)
455-455: LGTM!Also applies to: 585-585, 642-642, 1094-1094, 1113-1113
maki-ui/src/components/messages/tests.rs (1)
808-820: LGTM!Also applies to: 1109-1110, 1112-1134
plugins/todo_write/init.lua (1)
1-4: LGTM!Also applies to: 132-133, 148-151
maki-highlight/src/lib.rs (1)
11-12: LGTM!Also applies to: 113-113
maki-ui/src/components/code_view.rs (1)
1-2: LGTM!Also applies to: 13-13, 97-196, 455-535, 583-836
maki-lua/src/api/agent.rs (1)
4-7: LGTM!Also applies to: 42-43, 474-479, 489-504, 565-572, 753-753, 779-782, 900-922, 945-945
site/docs/content/lua-api/_index.md (1)
876-881: 📐 Maintainability & Code Quality | ⚡ Quick winReturned-fields list will likely render as one run-on paragraph.
The
elapsed_ms/current_tool/recent_tools/completed_count/donelines are indented but lack markdown list markers, so most renderers will collapse them into a single paragraph instead of a per-field list.📝 Proposed fix
Returns a table with: - `elapsed_ms` (integer): time since the session was created. - `current_tool` (string?): name of the tool currently running, if any. - `recent_tools` (table): names of the last few finished tools, oldest first. - `completed_count` (integer): total number of finished tools so far. - `done` (bool): true once the prompt has completed. + +- `elapsed_ms` (integer): time since the session was created. +- `current_tool` (string?): name of the tool currently running, if any. +- `recent_tools` (table): names of the last few finished tools, oldest first. +- `completed_count` (integer): total number of finished tools so far. +- `done` (bool): true once the prompt has completed.plugins/task/init.lua (3)
251-258: 🩺 Stability & Availability | ⚡ Quick win
do_poll's outcome fromgatheris silently discarded.Only
results[1](do_prompt) is checked; ifdo_pollerrors (e.g., insidepreview:update), it fails silently with no diagnostic and the preview simply stops updating with no trace of why.♻️ Suggested improvement
local results = maki.async.gather({ do_prompt, do_poll }) sess:close() local prompt_res = results[1] if not prompt_res.ok then error(prompt_res.err, 0) end + if not results[2].ok then + maki.log.warn("task preview poll failed: " .. tostring(results[2].err)) + end return prompt_res.value
90-105: LGTM!Also applies to: 118-124, 125-267, 269-286
106-116: 🎯 Functional CorrectnessHeader entries are already in the expected format. The extra braces are intentional here, so this line doesn’t need to change.
> Likely an incorrect or invalid review comment.
| fn add_recent(&self, tool: &str) { | ||
| let mut state = self.state.lock().unwrap_or_else(|e| e.into_inner()); | ||
| state.current = None; | ||
| state.completed_count += 1; | ||
| if state.recent.len() >= PROGRESS_MAX_RECENT { | ||
| state.recent.pop_front(); | ||
| } | ||
| state.recent.push_back(tool.to_owned()); | ||
| drop(state); | ||
| self.notify(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the agent loop can start multiple ToolStart events before their
# matching ToolDone, i.e. concurrent tool execution.
rg -n "ToolStart" maki-agent -C3Repository: w0wl0lxd/noon
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top-level files:"
git ls-files | sed -n '1,200p'
echo
echo "Relevant Rust files:"
git ls-files | rg 'agent\.rs|tool|progress|maki' -nRepository: w0wl0lxd/noon
Length of output: 6518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
echo "---"
git ls-files | sed -n '1,120p'Repository: w0wl0lxd/noon
Length of output: 3309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg 'agent\.rs|progress|tool|maki' -nRepository: w0wl0lxd/noon
Length of output: 696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Outline: noon-agent/src/agent/tool_dispatch.rs"
ast-grep outline noon-agent/src/agent/tool_dispatch.rs --view expanded || true
echo
echo "Outline: noon-agent/src/agent/run.rs"
ast-grep outline noon-agent/src/agent/run.rs --view expanded || true
echo
echo "Outline: noon-lua/src/api/agent.rs"
ast-grep outline noon-lua/src/api/agent.rs --view expanded || trueRepository: w0wl0lxd/noon
Length of output: 9306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ToolStart|ToolDone|ToolCall|tool dispatch|dispatch.*tool|current =" noon-agent/src noon-lua/src -C 3Repository: w0wl0lxd/noon
Length of output: 49028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "noon-agent/src/agent/tool_dispatch.rs (360-415)"
sed -n '360,415p' noon-agent/src/agent/tool_dispatch.rs | cat -n
echo
echo "noon-lua/src/api/agent.rs (660-726)"
sed -n '660,726p' noon-lua/src/api/agent.rs | cat -nRepository: w0wl0lxd/noon
Length of output: 4594
Track in-flight tool IDs before clearing current. process_tool_calls can run multiple tools in parallel, so clearing current on any ToolDone can hide another still-running tool and make the UI fall back too early. Consider tracking active tool IDs or a count instead of a single slot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@maki-lua/src/api/agent.rs` around lines 708 - 718, Update add_recent and the
surrounding process_tool_calls progress state to track in-flight tool IDs or an
active-tool count before clearing current. Only clear current and allow the UI
to fall back once all parallel tools have completed, while preserving
recent-tool recording and completion counting for each ToolDone.
1 participant
No description provided.