feat: Add support for scheduling header re-rendering#161
feat: Add support for scheduling header re-rendering#161carlos-algms merged 1 commit intocarlos-algms:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an optional on_after callback to P.invoke_hook and supplies it when invoking on_session_update, on_prompt_submit, and on_response_complete. SessionManager now coalesces and schedules header refreshes for headers defined as functions via a new _schedule_function_header_refresh method and a _header_refresh_scheduled guard. README examples were updated to store usage as data.update in per-tab state and to build headers using a pieces array with table.concat, including optional context usage and cost fields. Sequence Diagram(s)sequenceDiagram
participant User as User Event
participant SessionMgr as SessionManager
participant Hooks as Hook Registry
participant After as on_after callback
participant Config as Config.headers
participant Widget as ChatWidget
User->>+SessionMgr: emit session_update / prompt_submit / response_complete
SessionMgr->>+Hooks: P.invoke_hook(hook_name, data, on_after)
Hooks-->>-SessionMgr: handlers execute (via vim.schedule)
SessionMgr->>+After: on_after() (runs after handlers)
After-->>+SessionMgr: calls _schedule_function_header_refresh()
SessionMgr->>+Config: check headers for function entries
alt function headers found
SessionMgr->>+Widget: widget:render_header(panel_name) for each function header
Widget-->>-SessionMgr: rendered header applied
else no function headers
SessionMgr-->>-Config: no-op
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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)
lua/agentic/session_manager.lua (1)
265-271:⚠️ Potential issue | 🟠 MajorCoalesce header refreshes to avoid stream-time scheduling bursts
When
on_session_updateis configured, this path refreshes function headers for every update, includingagent_message_chunk/agent_thought_chunk. That can enqueue many render calls during streaming and cause avoidable UI churn.💡 Suggested coalescing patch
function SessionManager:_refresh_function_headers() + if self._function_header_refresh_pending then + return + end + self._function_header_refresh_pending = true + + vim.schedule(function() + self._function_header_refresh_pending = false + if not Config.headers then + return + end + + for panel_name, header_config in pairs(Config.headers) do + if type(header_config) == "function" then + self.widget:render_header(panel_name) + end + end + end) - if not Config.headers then - return - end - - for panel_name, header_config in pairs(Config.headers) do - if type(header_config) == "function" then - self.widget:render_header(panel_name) - end - end endAlso applies to: 410-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/session_manager.lua` around lines 265 - 271, The current call inside P.invoke_hook("on_session_update", ...) immediately calls self:_refresh_function_headers() for every update, causing many renders during streaming; change this to coalesce/debounce refreshes by replacing the direct call with a scheduled/guarded refresh: introduce a per-instance flag or timer (e.g., self._refresh_scheduled or use vim.defer_fn) and a helper like schedule_function_header_refresh() that sets the flag and defers a single self:_refresh_function_headers() call after a short delay (and clears the flag), and also skip scheduling when update.type is "agent_message_chunk" or "agent_thought_chunk"; apply the same coalescing change to the other occurrence around the 410-422 block so both code paths use schedule_function_header_refresh() instead of calling self:_refresh_function_headers() directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 714-715: The examples access optional fields without guards: wrap
accesses to data.update.cost, data.update.cost.amount and
data.update.cost.currency with nil checks (e.g., check data.update.cost exists
before reading amount/currency) and handle missing cost with a safe default or
skip logging; likewise check usage.size (and ensure it's nonzero) before
performing the division to compute per-unit values (avoid dividing by nil/0),
updating the README Lua hook snippets that reference data.update.cost.* and
usage.size accordingly.
---
Outside diff comments:
In `@lua/agentic/session_manager.lua`:
- Around line 265-271: The current call inside
P.invoke_hook("on_session_update", ...) immediately calls
self:_refresh_function_headers() for every update, causing many renders during
streaming; change this to coalesce/debounce refreshes by replacing the direct
call with a scheduled/guarded refresh: introduce a per-instance flag or timer
(e.g., self._refresh_scheduled or use vim.defer_fn) and a helper like
schedule_function_header_refresh() that sets the flag and defers a single
self:_refresh_function_headers() call after a short delay (and clears the flag),
and also skip scheduling when update.type is "agent_message_chunk" or
"agent_thought_chunk"; apply the same coalescing change to the other occurrence
around the 410-422 block so both code paths use
schedule_function_header_refresh() instead of calling
self:_refresh_function_headers() directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3b6b42e-adf9-4f46-8e74-b178bd396094
📒 Files selected for processing (2)
README.mdlua/agentic/session_manager.lua
00d7c2a to
199d6ac
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/agentic/session_manager.lua (1)
32-48:⚠️ Potential issue | 🔴 Critical
on_aftercallback is never invoked when no hook is configured.The
on_aftercallback is only called inside theif hook and type(hook) == "function"block. If the user hasn't configured any hooks (which is the default), the header refresh will never occur—defeating the PR's purpose.🐛 Proposed fix to ensure on_after is always called
function P.invoke_hook(hook_name, data, on_after) local hook = Config.hooks and Config.hooks[hook_name] if hook and type(hook) == "function" then vim.schedule(function() local ok, err = pcall(hook, data) if not ok then Logger.debug( string.format("Hook '%s' error: %s", hook_name, err) ) end if on_after then on_after() end end) + elseif on_after then + vim.schedule(on_after) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/session_manager.lua` around lines 32 - 48, P.invoke_hook currently only runs the on_after callback inside the branch where a hook exists and is a function, so when no hook is configured on_after is never called; update P.invoke_hook to always invoke on_after (wrapped in vim.schedule to match existing behavior) even when Config.hooks[hook_name] is nil or not a function—i.e., after checking/pcall-ing the hook (and logging via Logger.debug on error), add an else (or a subsequent unconditional check) that calls vim.schedule(function() on_after() end) when on_after is provided so header refresh always runs; keep the existing pcall and logging around the call to the hook.
♻️ Duplicate comments (1)
README.md (1)
798-804:⚠️ Potential issue | 🟡 MinorGuard against division by zero and nil cost access in example code.
Per session_manager.lua line 248,
costis optional inusage_update. Line 800 divides byusage.sizewithout checking for nil/zero, and lines 802-803 accessusage.cost.amountandusage.cost.currencywithout nil checks. Users copying this example will encounter runtime errors.📝 Proposed fix for safe access
local usage = vim.t.agentic_usage if usage ~= nil then - local pct = (usage.used / usage.size) * 100 - table.insert(pieces, ("Context: %.1f%%%% (%d/%d)"):format(pct, usage.used, usage.size)) + local used = tonumber(usage.used) or 0 + local size = tonumber(usage.size) or 0 + if size > 0 then + local pct = (used / size) * 100 + table.insert(pieces, ("Context: %.1f%%%% (%d/%d)"):format(pct, used, size)) + end if usage.cost ~= nil then table.insert(pieces, ("%.2f %s"):format(usage.cost.amount, usage.cost.currency)) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 798 - 804, The example code computes pct and accesses cost without guarding for nil/zero; update the block that reads local usage = vim.t.agentic_usage to first check that usage is non-nil and usage.size > 0 before computing pct (skip or show "N/A" if size is nil/zero) and only access usage.cost.amount / usage.cost.currency after confirming usage.cost is non-nil; adjust the table.insert calls that use pct and cost to use these safe values or omit the cost piece when absent (referencing the local usage variable and the pct/cost accesses in the snippet).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/agentic/session_manager.lua`:
- Around line 32-48: P.invoke_hook currently only runs the on_after callback
inside the branch where a hook exists and is a function, so when no hook is
configured on_after is never called; update P.invoke_hook to always invoke
on_after (wrapped in vim.schedule to match existing behavior) even when
Config.hooks[hook_name] is nil or not a function—i.e., after checking/pcall-ing
the hook (and logging via Logger.debug on error), add an else (or a subsequent
unconditional check) that calls vim.schedule(function() on_after() end) when
on_after is provided so header refresh always runs; keep the existing pcall and
logging around the call to the hook.
---
Duplicate comments:
In `@README.md`:
- Around line 798-804: The example code computes pct and accesses cost without
guarding for nil/zero; update the block that reads local usage =
vim.t.agentic_usage to first check that usage is non-nil and usage.size > 0
before computing pct (skip or show "N/A" if size is nil/zero) and only access
usage.cost.amount / usage.cost.currency after confirming usage.cost is non-nil;
adjust the table.insert calls that use pct and cost to use these safe values or
omit the cost piece when absent (referencing the local usage variable and the
pct/cost accesses in the snippet).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1f7e17a-22e0-4ef1-b1fa-6a46900c6730
📒 Files selected for processing (2)
README.mdlua/agentic/session_manager.lua
49a8c20 to
ca3beba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
798-808:⚠️ Potential issue | 🟡 MinorFix the copied usage/cost example before it renders
%%or errors on sparse cost data.Line 804 should use
%%, not%%%%, or the header prints a double percent sign. The example also still assumesusage.cost.amountandusage.cost.currencyexist, and nesting cost rendering undersize > 0hides cost-only updates.🛠️ Docs-safe snippet update
- if usage ~= nil then - local used = tonumber(usage.used) or 0 - local size = tonumber(usage.size) or 0 - if size > 0 then - local pct = (used / size) * 100 - table.insert(pieces, ("Context: %.1f%%%% (%d/%d)"):format(pct, used, size)) - if usage.cost ~= nil then - table.insert(pieces, ("%.2f %s"):format(usage.cost.amount, usage.cost.currency)) - end - end - end + if usage ~= nil then + local used = tonumber(usage.used) or 0 + local size = tonumber(usage.size) or 0 + if size > 0 then + local pct = (used / size) * 100 + table.insert(pieces, ("Context: %.1f%% (%d/%d)"):format(pct, used, size)) + end + + local cost = usage.cost + local amount = cost and tonumber(cost.amount) or nil + local currency = cost and cost.currency or nil + if amount ~= nil and currency ~= nil and currency ~= "" then + table.insert(pieces, ("%.2f %s"):format(amount, currency)) + end + endIn Lua 5.1 string.format, does "%%" render one percent sign while "%%%%" renders two?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 798 - 808, The snippet prints a double percent and can error on missing cost fields: in the block using local usage/used/size/pct and table.insert(pieces,...), change the format string from "Context: %.1f%%%% (%d/%d)" to use a single escaped percent ("%%") so it prints one percent, move the cost rendering out of the size > 0 branch so cost-only updates are shown, and guard access to usage.cost.amount and usage.cost.currency (check usage.cost and presence of amount/currency or use safe defaults) before calling table.insert to avoid nil errors; update references around pct, usage.cost, and table.insert accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/agentic/session_manager.lua`:
- Around line 267-282: The current branch prevents header refresh when
update.sessionUpdate is "agent_message_chunk" or "agent_thought_chunk", causing
user-triggered header changes in those hooks to remain stale; remove the
conditional so the on_after passed to P.invoke_hook always calls
self:_schedule_function_header_refresh(), and implement debouncing inside
_schedule_function_header_refresh (or wrap it with a debounce helper) to
throttle high-frequency streaming updates instead of suppressing them; reference
the update.sessionUpdate checks, the on_after closure around
P.invoke_hook("on_session_update", ...), and the
_schedule_function_header_refresh method when making these changes.
---
Duplicate comments:
In `@README.md`:
- Around line 798-808: The snippet prints a double percent and can error on
missing cost fields: in the block using local usage/used/size/pct and
table.insert(pieces,...), change the format string from "Context: %.1f%%%%
(%d/%d)" to use a single escaped percent ("%%") so it prints one percent, move
the cost rendering out of the size > 0 branch so cost-only updates are shown,
and guard access to usage.cost.amount and usage.cost.currency (check usage.cost
and presence of amount/currency or use safe defaults) before calling
table.insert to avoid nil errors; update references around pct, usage.cost, and
table.insert accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8b8176c-523b-4074-bc27-7f684c440477
📒 Files selected for processing (2)
README.mdlua/agentic/session_manager.lua
ca3beba to
e316ecd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/agentic/session_manager.lua (1)
36-48:⚠️ Potential issue | 🟠 Major
on_aftercallback is silently dropped when no hook is configured.If a user configures
headers.chatas a function but does not configurehooks.on_session_update, theon_aftercallback is never invoked because the entireifblock is skipped. This means function-based headers won't refresh for users who haven't set up hooks.🛠️ Proposed fix
if on_after then on_after() end end) +elseif on_after then + vim.schedule(on_after) endAdd an
elseifbranch to callon_after(wrapped invim.scheduleto maintain consistent async behavior) when no hook is configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/session_manager.lua` around lines 36 - 48, The current block only schedules and calls hook (with pcall) when hook exists, so on_after is never invoked if hooks.on_session_update is not configured; update the conditional in session_manager.lua to add an elseif branch that, when hook is nil but on_after is present, calls vim.schedule(function() on_after() end) (keep the existing pcall-wrapped scheduled call for when hook exists), referencing the existing symbols hook, hook_name, and on_after (and the headers.chat/hooks.on_session_update configuration) so function-based headers still trigger the on_after refresh even without a configured hook.
♻️ Duplicate comments (1)
README.md (1)
805-806: 🧹 Nitpick | 🔵 TrivialGuard
cost.amountandcost.currencybefore formatting.If
usage.costexists butamountorcurrencyare nil,string.format("%.2f", nil)will error. While the NOTE disclaims provider variations, a more defensive example would better demonstrate best practices.♻️ Suggested improvement
- if usage.cost ~= nil then - table.insert(pieces, ("%.2f %s"):format(usage.cost.amount, usage.cost.currency)) + local cost_amount = usage.cost and usage.cost.amount + local cost_currency = usage.cost and usage.cost.currency + if cost_amount ~= nil and cost_currency ~= nil then + table.insert(pieces, ("%.2f %s"):format(cost_amount, cost_currency)) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 805 - 806, The formatting call assumes usage.cost.amount and usage.cost.currency are non-nil which can crash; update the conditional around the table.insert to also check that usage.cost.amount and usage.cost.currency are not nil (or provide safe fallbacks) before calling ("%.2f %s"):format(...). Specifically, modify the block that references usage.cost and the table.insert(pieces, ("%.2f %s"):format(usage.cost.amount, usage.cost.currency)) so it only formats when both usage.cost.amount and usage.cost.currency exist (or use tostring/default values) to avoid string.format(nil) errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/agentic/session_manager.lua`:
- Around line 36-48: The current block only schedules and calls hook (with
pcall) when hook exists, so on_after is never invoked if hooks.on_session_update
is not configured; update the conditional in session_manager.lua to add an
elseif branch that, when hook is nil but on_after is present, calls
vim.schedule(function() on_after() end) (keep the existing pcall-wrapped
scheduled call for when hook exists), referencing the existing symbols hook,
hook_name, and on_after (and the headers.chat/hooks.on_session_update
configuration) so function-based headers still trigger the on_after refresh even
without a configured hook.
---
Duplicate comments:
In `@README.md`:
- Around line 805-806: The formatting call assumes usage.cost.amount and
usage.cost.currency are non-nil which can crash; update the conditional around
the table.insert to also check that usage.cost.amount and usage.cost.currency
are not nil (or provide safe fallbacks) before calling ("%.2f %s"):format(...).
Specifically, modify the block that references usage.cost and the
table.insert(pieces, ("%.2f %s"):format(usage.cost.amount, usage.cost.currency))
so it only formats when both usage.cost.amount and usage.cost.currency exist (or
use tostring/default values) to avoid string.format(nil) errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4a9eb5a-c3db-49c2-8597-eb0e3e34788e
📒 Files selected for processing (2)
README.mdlua/agentic/session_manager.lua
e316ecd to
503f939
Compare
This is valid, but that's the case before this too. Fixing this probably means using a timer ( |
503f939 to
c560f2f
Compare
carlos-algms
left a comment
There was a problem hiding this comment.
I guess this new method is fine.
Fix the pipeline, remove the on_after leftover and let's move on.
c560f2f to
a465950
Compare
a465950 to
daa8100
Compare
To do this efficiently, I added a schedule_header_refresh method to the session manager that batches header refreshes, preventing redundant updates if multiple hooks are invoked in quick succession. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
daa8100 to
321f32e
Compare
|
Rebased to resolve conflicts. |
Basically, hooks may need to trigger re-rendering of the different window headers if they're injecting new state that custom header functions will use. To do this efficiently, there should be a method on the session manager to refresh the headers efficiently so that hooks that run frequently don't cause excessive re-renders. To this, I added a `schedule_header_refresh` method that hooks can use by getting the current session and calling it from with. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
Basically, hooks may need to trigger re-rendering of the different window headers if they're injecting new state that custom header functions will use. To do this efficiently, there should be a method on the session manager to refresh the headers efficiently so that hooks that run frequently don't cause excessive re-renders. To this, I added a `schedule_header_refresh` method that hooks can use by getting the current session and calling it from with. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
Basically, hooks may need to trigger re-rendering of the different window headers if they're injecting new state that custom header functions will use. To do this efficiently, there should be a method on the session manager to refresh the headers efficiently so that hooks that run frequently don't cause excessive re-renders.
To this, I added a
schedule_header_refreshmethod that hooks can use by getting the current session and calling it from with. For example: