fix: switch provider and new session start fresh#175
Merged
carlos-algms merged 18 commits intomainfrom Apr 1, 2026
Merged
Conversation
…ailure - Start busy animation as soon as widget is created, not after ACP initialize completes - Detect provider failure (process crash, initialize error, cached dead client) and stop animation, write error to chat buffer so users can recover - Store _connection_error on SessionManager (per-session state, not on shared ACPClient) - Add on_session_ready callback for deferred work after session initialization - Clear session ready callbacks in _cancel_session to prevent stale closures
Bulk-replay ChatHistory messages into buffer with proper sender headers, used by provider switch to restore visual chat state. Temporarily swaps provider name per message so agent headers show the correct provider from history.
Prevent prompt submission when: - Session not yet created (waiting for ACP initialize) - Session restore in progress - Agent is generating response Notifies user of the reason via Logger.notify so they understand why submission was blocked.
Replace mutate-in-place switch with destroy-and-recreate: - Validate new provider available BEFORE destroying old session - Fully destroy old session via SessionRegistry - Create fresh session via registry - Register on_session_ready callback to restore state: - Replay chat history messages visually - Restore files and code selections - Open widget immediately if it was open before
Replace with destroy-and-recreate approach implemented in init.lua:apply_provider_switch(). SessionManager no longer mutates in-place for provider switches; instead, the registry fully destroys the old session and creates a fresh one with restored state via on_session_ready callback. Remove all switch_provider tests as method is no longer public.
The callback was being registered inside a callback to get_session_for_tab_page, which meant it was being registered AFTER the session callback had already been invoked. This caused the on_session_ready callback to fire before we had registered the restoration logic. Now we call get_session_for_tab_page directly, register the callback immediately, then open the widget. This ensures history restoration happens before the user sees the widget.
Tests demonstrate the bug: history is NOT restored after provider switch. RED phase of TDD cycle: - test_restores_chat_history_messages: Fails because new session has no messages - test_blocks_when_initializing: Fails because session isn't set up properly - test_blocks_when_generating: Fails because session isn't set up properly These tests will guide the fix for apply_provider_switch().
Tests verify that chat history is properly restored when switching providers: - History messages are restored to chat_history.messages - History is queued in history_to_send for next prompt - Messages are replayed visually in the chat buffer - Callback guards prevent restoration if session is replaced Also rename _history_to_send to history_to_send (public field) to allow external access from apply_provider_switch() in init.lua. Initialize the field in SessionManager:new() and clear it on session cancellation. All validations pass and tests confirm the provider switch implementation correctly preserves and restores chat context through the destroy-recreate cycle.
…witch Critical fix: history was being set in async callback, but user could submit prompt before callback fired, causing history to be lost. New approach: - Restore chat_history.messages and history_to_send IMMEDIATELY (sync) - Visual replay still async (happens when session ready, doesn't block user) - If user submits prompt before visual replay, history is still sent (already set) - When session ready callback fires, messages are replayed visually This ensures history is sent with the next prompt even if user submits before the async replay callback fires. Add debug logging to trace callback execution and session replacement.
Only set history_to_send immediately (sync) to ensure it's available if user submits prompt before async callbacks fire. Move the actual message restoration and visual replay to on_session_ready callback so it happens AFTER: - Agent becomes ready - Session is created - Welcome banner is written This prevents replaying messages onto a buffer that's about to be modified by the welcome message, and ensures proper timing with the busy animation.
|
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 change implements guarded provider switching in agentic.init (apply_provider_switch): it blocks switches when a session is initializing or generating, snapshots UI/session state, validates the target provider via AgentInstance.get_instance, destroys the old session, updates Config.provider, creates a new session, restores files/selections, and replays/restores chat history when the new session becomes ready. SessionManager gained connection-error handling, on_session_ready callbacks, can_submit_prompt gating, and changed submit behavior. MessageWriter gained replay_history_messages. Several new and updated tests plus docs for same-process async testing were added. Sequence Diagram(s)sequenceDiagram
participant User
participant Agentic as Agentic (init.lua)
participant SM as SessionManager
participant AI as AgentInstance
participant SR as SessionRegistry
participant MW as MessageWriter
participant Config
User->>Agentic: request switch_provider("NewProvider")
Agentic->>SM: inspect session (session_id, is_generating)
alt blocked (initializing or generating)
Agentic->>User: Logger.notify("blocked: initializing/generating")
else allowed
Agentic->>Agentic: snapshot state (history, files, selections, widget, tab)
Agentic->>AI: pcall AgentInstance.get_instance("NewProvider")
alt provider unavailable
Agentic->>User: Logger.notify("error: provider unavailable")
else provider available
Agentic->>SR: destroy old session for tab
Agentic->>Config: set provider = "NewProvider"
Agentic->>SR: create new session for tab
Agentic->>SM: set history_to_send(saved history)
Agentic->>SM: restore files & selections
SM->>SM: write welcome banner
SM->>SM: fire on_session_ready callbacks
SM->>MW: replay_history_messages(saved messages)
MW-->>SM: messages replayed
Agentic->>User: Logger.notify("switch complete")
end
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
carlos-algms
force-pushed
the
fix/destroy-sesion-to-create-new-fresh
branch
from
c7263a6 to
c8cb389
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lua/agentic/session_manager.lua (2)
957-977:⚠️ Potential issue | 🟠 MajorClear queued
on_session_ready()callbacks when cancelling.
_cancel_session()resetshistory_to_send, but it leaves_session_ready_callbacksbehind. If a switch queues a replay callback and the user starts/newor otherwise cancels before readiness, that stale closure will fire against the next successful session and replay the wrong history into it.Minimal fix
function SessionManager:_cancel_session() self._is_restoring_session = false + self._session_ready_callbacks = {} if self.session_id then🤖 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 957 - 977, SessionManager:_cancel_session currently resets history_to_send but leaves any queued _session_ready_callbacks intact, which can later replay into a new session; inside SessionManager:_cancel_session, clear the queued callbacks (e.g. set self._session_ready_callbacks = {} or nil) right after resetting session state so that any pending on_session_ready closures are dropped when the session is cancelled.
123-160:⚠️ Potential issue | 🟡 MinorAvoid scheduling the same connection-error path twice.
The
on_readyclosure already checksself.agent.state, and Lines 155-160 enqueue the same failure path again. With a cached agent that is already"error"or"disconnected", this appends the failure banner twice.🤖 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 123 - 160, Duplicate connection-error handling exists: the callback passed to AgentInstance.get_instance (the vim.schedule closure) already checks self.agent.state and calls self:_handle_connection_error(), so the later post-initialization block that again checks self.agent.state == "error" or "disconnected" and schedules self:_handle_connection_error() should be removed to avoid showing the failure banner twice; locate the callback supplied to AgentInstance.get_instance and the subsequent "Check for sync failure during ACPClient construction" block and delete the latter redundant check/schedule so only the get_instance closure handles the error path.
🤖 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/agentic.test.lua`:
- Around line 88-196: Add a new test that creates two tab pages and their
SessionManager instances (use SessionManager:new and
SessionRegistry.sessions[tab_page_id] for both), populate the first tab's
session.chat_history with messages and leave the second tab's session with
distinct messages/state, then call Agentic.switch_provider({ provider =
"NewProvider" }) only for the first tab (set Config.provider beforehand) and
after async waits assert that the first tab's session was updated
(session_id/history_to_send/chat_history restored) while the second tab's
session, its chat_history messages, session_id and any widget/ui-related flags
remain exactly unchanged; reference SessionManager, SessionRegistry,
Agentic.switch_provider, session.chat_history and history_to_send in the
assertions to ensure no cross-tab contamination.
- Around line 97-100: The test uses unconditional timeouts (vim.wait(100,
function() return false end)) which causes flakiness; replace those waits with
conditional waits that poll the real readiness signals — e.g., wait until
session.session_id is set for the agent ready -> new_session step, wait until
the replacement registry entry appears for registry-related assertions, and wait
until the restored history count increments for history restore checks; update
both occurrences (around the session_ready wait and the later registry/history
wait at the other location) to use vim.wait(timeout, function() return
<condition> end) where <condition> checks the appropriate variable/state.
In `@lua/agentic/init.lua`:
- Around line 248-273: The callback currently overwrites any messages submitted
early by replacing ready_session.chat_history.messages and
ready_session.history_to_send with the earlier saved_messages; instead, detect
if the early-submit queue was consumed/modified and merge rather than clobber.
In the new_session:on_session_ready handler, check
ready_session.chat_history.messages and ready_session.history_to_send (and
consider what SessionManager:_handle_input_submit does) and only prepend/apply
saved_messages that are not already present, or skip re-arming history_to_send
if it has been consumed/changed; update ready_session.chat_history.messages and
call message_writer:replay_history_messages only for the delta of missing
messages to avoid dropping the early prompt.
- Around line 240-279: Temporarily suppress the widget-open side effects while
restoring files/selections: set a flag on new_session (e.g.
new_session.suppress_widget_open = true) before calling
new_session.file_list:add(...) and new_session.code_selection:add(...), then
clear it after restores (and after the on_session_ready replay). Update the
session_manager callback that currently calls self.widget:show({ focus_prompt =
false }) when collections become non-empty to first check that not
session.suppress_widget_open (or similar) before calling widget:show, so
restores won’t re-open the widget when widget_was_open was false; keep existing
new_session:on_session_ready and new_session.widget:show usage intact.
- Around line 209-219: AgentInstance.get_instance(provider_name, ...) can raise,
so wrap the lookup inside a safe guard (e.g., pcall/xpcall) inside
switch_provider to avoid hard errors; call AgentInstance.get_instance within a
protected call, check the success flag and if false or result is nil call
Logger.notify("Provider '" .. provider_name .. "' not available.",
vim.log.levels.ERROR) and return without destroying the old session. Ensure you
reference AgentInstance.get_instance and switch_provider so the early-return
protection happens before any session teardown.
In `@lua/agentic/ui/message_writer.test.lua`:
- Around line 630-666: The test currently only checks that "Claude" appears
anywhere, which can hide bugs in writer:replay_history_messages; update the test
to explicitly assert the second agent provider header "Gemini" is rendered by
calling writer:replay_history_messages(messages), then using
get_all_lines()/table.concat to get buffer content and add an assertion that
content:match("Gemini") (or assert that a provider header appears adjacent to
the "from gemini" message) so the test fails if the second agent header is
incorrectly rendered as "Claude".
---
Outside diff comments:
In `@lua/agentic/session_manager.lua`:
- Around line 957-977: SessionManager:_cancel_session currently resets
history_to_send but leaves any queued _session_ready_callbacks intact, which can
later replay into a new session; inside SessionManager:_cancel_session, clear
the queued callbacks (e.g. set self._session_ready_callbacks = {} or nil) right
after resetting session state so that any pending on_session_ready closures are
dropped when the session is cancelled.
- Around line 123-160: Duplicate connection-error handling exists: the callback
passed to AgentInstance.get_instance (the vim.schedule closure) already checks
self.agent.state and calls self:_handle_connection_error(), so the later
post-initialization block that again checks self.agent.state == "error" or
"disconnected" and schedules self:_handle_connection_error() should be removed
to avoid showing the failure banner twice; locate the callback supplied to
AgentInstance.get_instance and the subsequent "Check for sync failure during
ACPClient construction" block and delete the latter redundant check/schedule so
only the get_instance closure handles the error path.
🪄 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: 0849859f-8b46-4836-8f49-98c7ecc41005
📒 Files selected for processing (6)
lua/agentic/agentic.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lualua/agentic/ui/message_writer.lualua/agentic/ui/message_writer.test.lua
💤 Files with no reviewable changes (1)
- lua/agentic/session_manager.test.lua
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/agentic.test.lua`:
- Around line 191-194: The test currently inspects only
logger_notify_stub.calls[1][1], which is fragile; change the assertion in the
tests referencing logger_notify_stub (e.g., the block around the assertions at
lines with logger_notify_stub and msg) to check the last call or any call that
matches the pattern instead of always using index 1 — e.g., retrieve the last
call via logger_notify_stub.calls[`#logger_notify_stub.calls`][1] or iterate the
calls to assert a matching message contains "[Ii]nitializ", and apply the same
fix for the other occurrence around lines 212-214.
- Around line 248-364: The test opens a new tab with vim.cmd("tabnew") and only
calls vim.cmd("tabclose") at the test end, so if an assertion fails the tab
remains open and pollutes later tests; move cleanup into an after_each() block
that always runs and ensure it closes any tabs created by the test (track
tab2_id created after vim.cmd("tabnew")), restores current tab (use
vim.api.nvim_set_current_tabpage(tab1_id) if needed), and removes any test
entries from SessionRegistry.sessions (e.g., delete
SessionRegistry.sessions[tab2_id]) and resets any SessionManager state created
in the test; implement this in the lua/agentic/agentic.test.lua file by adding
an after_each that references tab2_id, vim.cmd("tabclose"),
vim.api.nvim_set_current_tabpage, and SessionRegistry to guarantee cleanup even
on failures.
- Around line 99-102: The cleanup loop mutates SessionRegistry.sessions while
iterating with pairs(), which can skip entries; change the teardown to first
collect all tab IDs into a temporary array (e.g., iterate
pairs(SessionRegistry.sessions) and append each tab_id to a list) and then
iterate that list calling SessionRegistry.destroy_session(tab_id) for each;
reference the SessionRegistry.sessions table and the
SessionRegistry.destroy_session(tab_id) function when making this change.
In `@tests/AGENTS.md`:
- Around line 649-657: The example test uses vim.schedule(...) then
vim.uv.sleep(10), which blocks the event loop so the scheduled callback never
runs; update the example to use an event-pumping wait instead (e.g., replace
vim.uv.sleep(...) with vim.wait(...) or another mechanism that allows
vim.schedule callbacks to execute) so the local result assigned inside the
vim.schedule callback is set before assert.equal('expected', result) runs;
specifically modify the test function around vim.schedule, result, and the sleep
call to use vim.wait (or an equivalent event-loop-friendly wait) so the
assertion is reliable.
🪄 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: fd3a2c03-dd27-4bfd-b913-0f60f241852b
📒 Files selected for processing (2)
lua/agentic/agentic.test.luatests/AGENTS.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/buf_helpers.lua`:
- Around line 68-76: The code sets opts.buf when vim.fn.has("nvim-0.12") == 1
but vim.keymap.set does not accept opts.buf; change the branch so both Neovim
>=0.12 and older branches set opts.buffer = bufnr (keep the comment about
dropping the fallback), i.e., replace the assignment to opts.buf in the
vim.fn.has("nvim-0.12") branch with opts.buffer = bufnr so that the mapping
options object (opts) uses the correct key for vim.keymap.set; update any nearby
diagnostic comment that referenced opts.buf if present.
🪄 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: 52587b46-4e10-44d0-988d-a2339a8ad25a
📒 Files selected for processing (1)
lua/agentic/utils/buf_helpers.lua
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lua/agentic/init.lua (2)
258-274:⚠️ Potential issue | 🟠 MajorDon't clobber history after an early submit.
Line 256 intentionally seeds
history_to_sendbefore the ready callback. If the user submits before this callback runs, Lines 268-269 resetchat_history.messagesandhistory_to_sendback tosaved_messages, which can drop the new prompt and re-arm the old queue. Restore only when the ready session is still untouched, otherwise merge the missing saved items instead of replacing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/init.lua` around lines 258 - 274, The current ready callback unconditionally overwrites ready_session.chat_history.messages and ready_session.history_to_send with saved_messages which can clobber a user-submitted prompt that was seeded earlier; change the logic in new_session:on_session_ready to detect if ready_session.chat_history.messages or ready_session.history_to_send already contains new entries (e.g., non-empty or length > `#saved_messages`) and, if so, merge missing items from saved_messages into the existing arrays instead of replacing them, otherwise perform the replace; keep the replay via ready_session.message_writer:replay_history_messages but ensure it only replays the merged/unique messages to avoid duplicating the user prompt.
241-247:⚠️ Potential issue | 🟠 MajorSuppress widget-open side effects while restoring context.
file_list:add()andcode_selection:add()can reenter the normal “context became non-empty” path, so this restore can reopen a widget that was closed before the switch. Gate those callbacks during restore and only callwidget:show()whenwidget_was_openwas true.Also applies to: 277-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/init.lua` around lines 241 - 247, The restore loops call new_session.file_list:add(...) and new_session.code_selection:add(...), which trigger normal "context became non-empty" callbacks and can reopen widgets; wrap the restore in a suppression/gating flag (e.g., set restoring_context = true on the session or a local suppression on the widget callback), perform the add() calls, then clear the flag and, after restore, only invoke widget:show() for widgets where widget_was_open was true; apply the same suppression pattern to the other restore block that uses new_session.file_list:add/new_session.code_selection:add so callbacks are no-ops during restore and side effects only occur conditionally afterward.lua/agentic/agentic.test.lua (1)
205-208:⚠️ Potential issue | 🟡 MinorMatch the guard notification by content, not by fixed index.
These assertions get brittle if setup or session creation emits another
Logger.notify()first. Inspect the last call or scanlogger_notify_stub.callsfor the expected substring instead of assuming the guard message is alwayscalls[1].Also applies to: 226-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/agentic.test.lua` around lines 205 - 208, The test currently assumes the guard init message is in logger_notify_stub.calls[1]; instead iterate/search logger_notify_stub.calls for the expected substring (e.g., "[Ii]nitializ") or use the last call (logger_notify_stub.calls[`#logger_notify_stub.calls`]) to assert the guard notification by content rather than fixed index; update both occurrences referencing logger_notify_stub so the assertion scans calls for a msg that matches the initialization regex.
🤖 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/agentic.test.lua`:
- Around line 165-166: The test currently pre-seeds Config.provider to the
expected new value before calling Agentic.switch_provider, which masks
regressions; update the tests that set Config.provider = "NewProvider" (and the
analogous "SwitchedProvider" case) to instead preserve the original
Config.provider in setup, call Agentic.switch_provider({ provider =
"NewProvider" }), and then assert that Config.provider has changed to
"NewProvider" after the call (and likewise for "SwitchedProvider"); use the
Config.provider and Agentic.switch_provider identifiers to locate and modify the
two test cases.
---
Duplicate comments:
In `@lua/agentic/agentic.test.lua`:
- Around line 205-208: The test currently assumes the guard init message is in
logger_notify_stub.calls[1]; instead iterate/search logger_notify_stub.calls for
the expected substring (e.g., "[Ii]nitializ") or use the last call
(logger_notify_stub.calls[`#logger_notify_stub.calls`]) to assert the guard
notification by content rather than fixed index; update both occurrences
referencing logger_notify_stub so the assertion scans calls for a msg that
matches the initialization regex.
In `@lua/agentic/init.lua`:
- Around line 258-274: The current ready callback unconditionally overwrites
ready_session.chat_history.messages and ready_session.history_to_send with
saved_messages which can clobber a user-submitted prompt that was seeded
earlier; change the logic in new_session:on_session_ready to detect if
ready_session.chat_history.messages or ready_session.history_to_send already
contains new entries (e.g., non-empty or length > `#saved_messages`) and, if so,
merge missing items from saved_messages into the existing arrays instead of
replacing them, otherwise perform the replace; keep the replay via
ready_session.message_writer:replay_history_messages but ensure it only replays
the merged/unique messages to avoid duplicating the user prompt.
- Around line 241-247: The restore loops call new_session.file_list:add(...) and
new_session.code_selection:add(...), which trigger normal "context became
non-empty" callbacks and can reopen widgets; wrap the restore in a
suppression/gating flag (e.g., set restoring_context = true on the session or a
local suppression on the widget callback), perform the add() calls, then clear
the flag and, after restore, only invoke widget:show() for widgets where
widget_was_open was true; apply the same suppression pattern to the other
restore block that uses new_session.file_list:add/new_session.code_selection:add
so callbacks are no-ops during restore and side effects only occur conditionally
afterward.
🪄 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: 1ec45fd6-40b1-416c-a70b-4eb28730796f
📒 Files selected for processing (4)
lua/agentic/agentic.test.lualua/agentic/init.lualua/agentic/ui/message_writer.test.lualua/agentic/utils/buf_helpers.lua
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lua/agentic/agentic.test.lua (1)
163-167: 🧹 Nitpick | 🔵 TrivialConsider not pre-seeding Config.provider to the target value.
Setting
Config.provider = "NewProvider"before callingswitch_provider({ provider = "NewProvider" })means the test would still pass if the implementation stopped updatingConfig.providercorrectly. For stronger regression detection, keep the original provider in setup and assert the change afterward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/agentic.test.lua` around lines 163 - 167, The test pre-seeds Config.provider to "NewProvider" which masks failures in Agentic.switch_provider; change the setup so Config.provider is left at its original/different value (or explicitly set to something not "NewProvider"), call Agentic.switch_provider({ provider = "NewProvider" }), and then assert that Config.provider has been updated to "NewProvider" (referencing Config.provider and Agentic.switch_provider in the test to locate the change).lua/agentic/init.lua (1)
248-254:⚠️ Potential issue | 🟡 MinorFile/selection restores may inadvertently open the widget.
The
file_list:add()andcode_selection:add()calls trigger the callbacks wired inSessionManager(Lines 183-206 in session_manager.lua), which callwidget:show()when collections become non-empty. Ifwidget_was_openwasfalse, this could unexpectedly open the widget during restore.Consider suppressing the widget-open side effect during restore, e.g., by setting a flag on the session to skip the
widget:show()call in the callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/init.lua` around lines 248 - 254, The restore loop calling new_session.file_list:add(...) and new_session.code_selection:add(...) triggers SessionManager callbacks that call widget:show(), so suppress that side effect during restore by adding a temporary flag on the session (e.g., new_session.suppress_widget_on_restore = true) before the loops and clearing it after; update the SessionManager callbacks that currently call widget:show() to check session.suppress_widget_on_restore (or a similarly named property) and skip calling widget:show() when the flag is set. Ensure the flag is set on the same session object used in the restore (new_session) and is removed or reset immediately after restore completes.
🤖 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/ui/message_writer.test.lua`:
- Around line 710-723: The fixture for testing writer:replay_history_messages
uses a top-level tool_call_id/kind/status/body instead of nesting them under
msg.tool_call, so the test doesn't exercise the tool-call replay path; update
the message shape so the message has type = "tool_call" and includes a tool_call
table (e.g., msg.tool_call = { id = "tc-1", kind = "read", status = "completed",
body = { "file content" }, file_path = "test.txt", provider_name = "Claude" })
so replay_history_messages and its use of msg.tool_call actually run the
tool-call branch.
---
Duplicate comments:
In `@lua/agentic/agentic.test.lua`:
- Around line 163-167: The test pre-seeds Config.provider to "NewProvider" which
masks failures in Agentic.switch_provider; change the setup so Config.provider
is left at its original/different value (or explicitly set to something not
"NewProvider"), call Agentic.switch_provider({ provider = "NewProvider" }), and
then assert that Config.provider has been updated to "NewProvider" (referencing
Config.provider and Agentic.switch_provider in the test to locate the change).
In `@lua/agentic/init.lua`:
- Around line 248-254: The restore loop calling new_session.file_list:add(...)
and new_session.code_selection:add(...) triggers SessionManager callbacks that
call widget:show(), so suppress that side effect during restore by adding a
temporary flag on the session (e.g., new_session.suppress_widget_on_restore =
true) before the loops and clearing it after; update the SessionManager
callbacks that currently call widget:show() to check
session.suppress_widget_on_restore (or a similarly named property) and skip
calling widget:show() when the flag is set. Ensure the flag is set on the same
session object used in the restore (new_session) and is removed or reset
immediately after restore completes.
🪄 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: ca934064-6e95-4695-ab41-41deeda0924f
📒 Files selected for processing (6)
lua/agentic/agentic.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lualua/agentic/ui/chat_widget.lualua/agentic/ui/message_writer.test.lua
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.
Uh oh!
There was an error while loading. Please reload this page.