fix: is_generating was stale and could make the chat unresponsive#178
fix: is_generating was stale and could make the chat unresponsive#178carlos-algms merged 11 commits intomainfrom
Conversation
Prevents is_generating from staying true after session cancel, which blocked user interaction on restore, new session, and provider switch.
Previously, stop_generation() sent session/cancel but never set is_generating = false. If the provider ignored the cancel or crashed, the flag stayed stuck true forever, blocking all user interaction. Also stops the spinner immediately instead of waiting for the callback that may never arrive.
- Prevents orphaned callbacks when provider crashes mid-generation - send_prompt, create_session, load_session callbacks now receive proper error on disconnect instead of hanging forever
…rompt guard Allows users to escape stuck is_generating=true state by typing /new, even when the generation guard would otherwise block all input.
…change Captures request_session_id before send_prompt and skips the finish message + stop-animation if session_id changed by the time the response arrives (cancel/restore/new session).
…ives nil
Fixes invalid ACP schema sent when stop_generation() cancels pending permissions.
Previously, nil option_id produced { outcome = "selected", optionId = nil }
which violates the schema. Now sends { outcome = "cancelled" } instead.
Prevents a stale callback from session A incorrectly clearing is_generating on session B if B is mid-generation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds Sequence Diagram(s)mermaid Client->>ACPClient: send_prompt(request, callback) ACPClient->>ACPClient: on_state_change("disconnected"/"error") Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
The drain fires from a libuv exit callback (uv.spawn on_exit), not from vim's main loop. Callbacks like create_session access Neovim APIs directly without vim.schedule, which would cause "fast event" errors. Wrapping in vim.schedule ensures safety.
`{ outcome = "selected", optionId = nil }` serializes to
`{ "outcome": "selected" }` in JSON — nil fields are omitted.
The original code was correct.
![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
🤖 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`:
- Line 983: The _cancel_session() path only resets self.is_generating = false
but doesn't stop the status animation like Agentic.stop_generation() does;
update _cancel_session() to also call self.status_animation:stop() (or check for
existence before calling) so it mirrors the behavior of
Agentic.stop_generation() and ensures the animation is explicitly stopped
whenever generation is cancelled.
🪄 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: 968c70b1-6a0b-4713-b90c-e70e61623b64
📒 Files selected for processing (6)
lua/agentic/acp/acp_client.lualua/agentic/acp/acp_client.test.lualua/agentic/agentic.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/acp/acp_client.test.lua`:
- Line 287: The LuaCATS diagnostic directive is missing a space after the
triple-dash; update the annotation string '---@diagnostic disable-next-line:
duplicate-set-field' to follow the project's style by inserting a space so it
reads '--- `@diagnostic` disable-next-line: duplicate-set-field' (adjust any other
similar annotations in the same file if present) to ensure consistent LuaCATS
formatting.
In `@lua/agentic/session_manager.lua`:
- Line 603: The pattern match for the `/new` command is too permissive and will
match inputs like `/newabc`; update the check in session_manager.lua that
currently uses input_text:match("^/new%s*") to require a token boundary so only
real `/new` invocations trigger new_session(); e.g., change to a pattern that
demands end or whitespace such as input_text:match("^/new(%s.*)?$") (affects the
block that calls new_session() using the input_text variable).
In `@lua/agentic/session_manager.test.lua`:
- Around line 815-849: The test for SessionManager:_cancel_session only asserts
session.is_generating is false; update the test to also assert that the
session.status_animation.stop method was called — locate the test that
constructs the session table and calls session:_cancel_session() (uses
SessionManager._cancel_session and local session variable) and replace or
augment the status_animation.stop stub with a spy (or wrap it) so you can assert
it was invoked after calling session:_cancel_session().
🪄 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: 34c621fe-938d-4ea2-b3e7-0945f500ef02
📒 Files selected for processing (4)
lua/agentic/acp/acp_client.lualua/agentic/acp/acp_client.test.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
1 participant
No description provided.