From aadc79068a850653d109f3e478b7258b832246b0 Mon Sep 17 00:00:00 2001 From: phanium <91544758+phanen@users.noreply.github.com> Date: Sun, 28 Jun 2026 14:03:58 +0800 Subject: [PATCH] fix(animation): spinner now driven by server status, not job_count Problem: 1. Spinner flashed on every UI open. The gate was jobs.is_running(), which every HTTP request bumps (including pure queries), so toggle on issued 3-4 queries and the spinner blinked. 2. Spinner never appeared after attach or toggle off-then-on. SSE cannot replay past events, and toggle off wiped the only local copy of status_data. 3. Spinner could stay on forever after the model finished: a. sync's GET response, stale by the time it landed, overwrote the cache with a busy snapshot and started the spinner against an already-idle server (which emits no follow-up). b. SSE idle before set_active was skipped (active_session guard), so replay read the stale busy. Solution: - Gate the spinner on a new `_should_animate()`: status_data is non-idle AND tagged with the active session's sessionID. - Per-session `last_status_map` (always a table) holds the cache. SSE writes unconditionally; sync merges only missing entries (SSE wins). - New `OpencodeApiClient:list_session_status` + `M.sync_from_server()` hydrate on setup and replay to status_data. - `M.refresh()` is the single entry point that reconciles the running state with status_data and active_session. Tests: - `does not regress to busy when sync returns a stale snapshot after SSE idle` covers race 3a. - `replays the active session from the cache (handles sync-before_set_active)` covers race 3b. - `merges the response into the cache (only fills missing entries)` asserts SSE-preserved entries survive sync. - `does not leave a stale spinner running when the model finishes during a hide` covers the toggle off / model finishes / toggle on scenario. --- lua/opencode/api_client.lua | 7 + lua/opencode/types.lua | 7 + lua/opencode/ui/loading_animation.lua | 109 +++++-- tests/unit/loading_animation_spec.lua | 409 +++++++++++++++++++++++--- 4 files changed, 472 insertions(+), 60 deletions(-) diff --git a/lua/opencode/api_client.lua b/lua/opencode/api_client.lua index c1eb97ab..c816ee07 100644 --- a/lua/opencode/api_client.lua +++ b/lua/opencode/api_client.lua @@ -195,6 +195,13 @@ function OpencodeApiClient:list_sessions(directory) return self:_call('/session', 'GET', nil, { directory = directory }) end +--- List the current status of all sessions in a workspace. +--- @param directory string|nil Directory path +--- @return Promise<{[string]: OpencodeSessionStatusInfo}> +function OpencodeApiClient:list_session_status(directory) + return self:_call('/session/status', 'GET', nil, { directory = directory }) +end + --- List sessions across all projects (experimental global endpoint). --- Bypasses _call's automatic directory injection so the server returns all --- directories instead of being filtered to the current cwd. diff --git a/lua/opencode/types.lua b/lua/opencode/types.lua index 4748d8b3..7c01caec 100644 --- a/lua/opencode/types.lua +++ b/lua/opencode/types.lua @@ -775,3 +775,10 @@ ---@class OpencodeSelectionRange ---@field start number Starting line number (inclusive) ---@field stop number Ending line number (inclusive) + +---@class OpencodeSessionStatusInfo +---@field type 'idle'|'busy'|'retry' Current status of the session +---@field message? string Human-readable detail (populated for `retry`) +---@field attempt? number Retry attempt counter (populated for `retry`) +---@field next? number Server-side timestamp of the next retry (populated for `retry`) +---@field action? table Optional retry action metadata (populated for `retry`) diff --git a/lua/opencode/ui/loading_animation.lua b/lua/opencode/ui/loading_animation.lua index 44da2f86..3cb6b102 100644 --- a/lua/opencode/ui/loading_animation.lua +++ b/lua/opencode/ui/loading_animation.lua @@ -8,12 +8,14 @@ M._animation = { frames = nil, text = 'Thinking... ', status_data = nil, + status_session_id = nil, current_frame = 1, timer = nil, fps = 10, extmark_id = nil, ns_id = vim.api.nvim_create_namespace('opencode_loading_animation'), status_event_manager = nil, + last_status_map = {}, } ---@param status table|nil @@ -90,20 +92,45 @@ function M.on_session_status(properties) return end - local active_session = state.active_session - if active_session and active_session.id and properties.sessionID ~= active_session.id then + if not properties.sessionID or not properties.status then return end - M._animation.status_data = properties.status + M._animation.last_status_map[properties.sessionID] = properties.status + + local active_session = state.active_session + if active_session and active_session.id == properties.sessionID then + M._animation.status_data = properties.status + M._animation.status_session_id = properties.sessionID + M.refresh() + end M.render(state.windows) end -local function on_active_session_change(_, new_session, old_session) +local function replay_status_for(session_id) + local status = M._animation.last_status_map[session_id] + if not status then + return + end + local active_session = state.active_session + if not active_session or active_session.id ~= session_id then + return + end + M._animation.status_data = status + M._animation.status_session_id = session_id + M.refresh() + M.render(state.windows) +end + +M._on_active_session_change = function(_, new_session, old_session) local new_id = new_session and new_session.id local old_id = old_session and old_session.id - if new_id ~= old_id then + if old_id and old_id ~= new_id then M._animation.status_data = nil + M._animation.status_session_id = nil + end + if new_id then + replay_status_for(new_id) end end @@ -138,8 +165,9 @@ M.render = vim.schedule_wrap(function(windows) return false end - if not state.jobs.is_running() then - M.stop() + M.refresh() + + if not M.is_running() then return false end @@ -167,8 +195,8 @@ function M._start_animation_timer(windows) interval = interval, on_tick = function() M._animation.current_frame = M._next_frame() - M.render(state.windows) - if state.jobs.is_running() then + M.render(windows) + if M._should_animate() then return true else M.stop() @@ -199,41 +227,84 @@ end function M.stop() M._clear_animation_timer() M._animation.current_frame = 1 - M._animation.status_data = nil if state.windows and state.windows.footer_buf and vim.api.nvim_buf_is_valid(state.windows.footer_buf) then pcall(vim.api.nvim_buf_clear_namespace, state.windows.footer_buf, M._animation.ns_id, 0, -1) end end +function M._should_animate() + local status = M._animation.status_data + if not status or status.type == 'idle' then + return false + end + local active_session = state.active_session + if not active_session then + return false + end + return M._animation.status_session_id == active_session.id +end + +function M.sync_from_server() + local api_client = state.api_client + if not api_client or not api_client.list_session_status then + return + end + + api_client + :list_session_status(state.current_cwd or vim.fn.getcwd()) + :and_then(function(status_map) + if type(status_map) ~= 'table' then + return + end + for session_id, status in pairs(status_map) do + if not M._animation.last_status_map[session_id] then + M._animation.last_status_map[session_id] = status + end + end + local active_session = state.active_session + if active_session then + replay_status_for(active_session.id) + end + end) + :catch(function(err) + require('opencode.log').debug('loading_animation.sync_from_server failed: %s', tostring(err)) + end) +end + function M.is_running() return M._animation.timer ~= nil end -local function on_running_change(_, new_value) +function M.refresh() if not state.windows then return end - - if not M.is_running() and new_value and new_value > 0 then - M.start(state.windows) - else + if M._should_animate() then + if not M.is_running() then + M.start(state.windows) + end + elseif M.is_running() then M.stop() end end function M.setup() - state.store.subscribe('job_count', on_running_change) - state.store.subscribe('active_session', on_active_session_change) + state.store.subscribe('job_count', M.refresh) + state.store.subscribe('active_session', M._on_active_session_change) state.store.subscribe('event_manager', on_event_manager_change) subscribe_session_status_event(state.event_manager) + M.sync_from_server() end function M.teardown() - state.store.unsubscribe('job_count', on_running_change) - state.store.unsubscribe('active_session', on_active_session_change) + state.store.unsubscribe('job_count', M.refresh) + state.store.unsubscribe('active_session', M._on_active_session_change) state.store.unsubscribe('event_manager', on_event_manager_change) unsubscribe_session_status_event(M._animation.status_event_manager) + M._animation.last_status_map = {} M._animation.status_data = nil + M._animation.status_session_id = nil + M._clear_animation_timer() end return M diff --git a/tests/unit/loading_animation_spec.lua b/tests/unit/loading_animation_spec.lua index eae65762..5865b180 100644 --- a/tests/unit/loading_animation_spec.lua +++ b/tests/unit/loading_animation_spec.lua @@ -1,57 +1,384 @@ local state = require('opencode.state') local loading_animation = require('opencode.ui.loading_animation') +local stub = require('luassert.stub') +local assert = require('luassert') -describe('loading_animation status text', function() - local original_time +local function reset() + if loading_animation._animation.timer then + loading_animation._animation.timer:stop() + loading_animation._animation.timer = nil + end + vim.wait(0) -- drain any pending vim.schedule emits from prior tests + state.jobs.set_count(0) + state.session.clear_active() + vim.wait(0) -- drain the clear_active emit + state.store.set_raw('windows', nil) + loading_animation._animation.status_data = nil + loading_animation._animation.status_session_id = nil + loading_animation._animation.last_status_map = {} + loading_animation._animation.current_frame = 1 + loading_animation._animation.extmark_id = nil +end - before_each(function() - original_time = os.time - loading_animation._animation.status_data = nil - state.session.clear_active() +describe('loading_animation', function() + before_each(reset) + after_each(reset) + + describe('_format_status_text', function() + it('returns the spinner text for busy', function() + assert.are.equal('Thinking... ', loading_animation._format_status_text({ type = 'busy' })) + end) + + it('returns nil for idle', function() + assert.is_nil(loading_animation._format_status_text({ type = 'idle' })) + end) + + it('formats retry with attempt and seconds-until-next', function() + local text = loading_animation._format_status_text({ + type = 'retry', + attempt = 2, + message = 'Provider overloaded', + next = os.time() * 1000 + 5000, + }) + assert.is_truthy(text:find('Provider overloaded')) + assert.is_truthy(text:find('retry 2')) + assert.is_truthy(text:find('in 5s')) + end) + end) + + describe('_should_animate', function() + it('returns false when status_data is nil', function() + assert.is_false(loading_animation._should_animate()) + end) + + it('returns false when status is idle', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation._animation.status_data = { type = 'idle' } + loading_animation._animation.status_session_id = 'ses_a' + assert.is_false(loading_animation._should_animate()) + end) + + it('returns false when there is no active session', function() + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + assert.is_false(loading_animation._should_animate()) + end) + + it('returns true when busy on the active session', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + assert.is_true(loading_animation._should_animate()) + end) + + it('returns false when busy on a different session', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_b' + assert.is_false(loading_animation._should_animate()) + end) + end) + + describe('M.refresh', function() + it('starts the spinner when should_animate transitions to true', function() + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + + loading_animation.refresh() + + assert.is_true(loading_animation.is_running()) + end) + + it('stops the spinner when should_animate transitions to false', function() + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + loading_animation.refresh() -- start it + assert.is_true(loading_animation.is_running()) + + state.session.clear_active() -- now should_animate is false + loading_animation.refresh() + + assert.is_false(loading_animation.is_running()) + end) + + it('is a no-op without state.windows', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + + loading_animation.refresh() + + assert.is_false(loading_animation.is_running()) + end) end) - after_each(function() - os.time = original_time - loading_animation._animation.status_data = nil - state.session.clear_active() + describe('on_session_status (SSE)', function() + it('updates the cache for any session, active or not', function() + loading_animation.on_session_status({ + sessionID = 'ses_a', + status = { type = 'busy' }, + }) + loading_animation.on_session_status({ + sessionID = 'ses_b', + status = { type = 'idle' }, + }) + + assert.are.equal('busy', loading_animation._animation.last_status_map.ses_a.type) + assert.are.equal('idle', loading_animation._animation.last_status_map.ses_b.type) + end) + + it('mirrors status_data only for the active session', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation.on_session_status({ + sessionID = 'ses_b', + status = { type = 'busy' }, + }) + assert.is_nil(loading_animation._animation.status_data) + + loading_animation.on_session_status({ + sessionID = 'ses_a', + status = { type = 'busy' }, + }) + assert.are.equal('busy', loading_animation._animation.status_data.type) + end) + + it('starts the spinner when busy arrives for the active session', function() + local start_stub = stub(loading_animation, 'start') + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + + loading_animation.on_session_status({ + sessionID = 'ses_a', + status = { type = 'busy' }, + }) + + assert.stub(start_stub).was_called(1) + start_stub:revert() + end) + + it('does not start the spinner when busy arrives for a non-active session', function() + local start_stub = stub(loading_animation, 'start') + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + + loading_animation.on_session_status({ + sessionID = 'ses_other', + status = { type = 'busy' }, + }) + + assert.stub(start_stub).was_not_called() + start_stub:revert() + end) + + it('stops the spinner when idle arrives for the active session', function() + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + loading_animation.refresh() + assert.is_true(loading_animation.is_running()) + + loading_animation.on_session_status({ + sessionID = 'ses_a', + status = { type = 'idle' }, + }) + + assert.is_false(loading_animation.is_running()) + end) + + it('also animates for retry (not just busy)', function() + local start_stub = stub(loading_animation, 'start') + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + + loading_animation.on_session_status({ + sessionID = 'ses_a', + status = { type = 'retry', message = 'overloaded', attempt = 1, next = 0 }, + }) + + assert.stub(start_stub).was_called(1) + start_stub:revert() + end) end) - it('renders busy as thinking text', function() - local text = loading_animation._format_status_text({ type = 'busy' }) - assert.are.equal('Thinking... ', text) + describe('on_active_session_change', function() + it('replays the active session from the cache (handles sync-before-set_active)', function() + loading_animation._animation.last_status_map.ses_x = { type = 'busy' } + state.store.subscribe('active_session', loading_animation._on_active_session_change) + + state.session.set_active({ id = 'ses_x' }) + vim.wait(200, function() + return loading_animation._animation.status_data ~= nil + end) + + assert.are.equal('busy', loading_animation._animation.status_data.type) + assert.are.equal('ses_x', loading_animation._animation.status_session_id) + end) + + it('clears status_data on actual session switch', function() + state.session.set_active({ id = 'ses_old' }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_old' + state.store.subscribe('active_session', loading_animation._on_active_session_change) + + state.session.set_active({ id = 'ses_new' }) + vim.wait(200, function() + return loading_animation._animation.status_data == nil + or loading_animation._animation.status_session_id == 'ses_new' + end) + + assert.is_nil(loading_animation._animation.status_data) + assert.is_nil(loading_animation._animation.status_session_id) + end) + + it('keeps status_data on first assignment (nil -> X)', function() + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_x' + state.store.subscribe('active_session', loading_animation._on_active_session_change) + + state.session.set_active({ id = 'ses_x' }) + vim.wait(200, function() + return loading_animation._animation.status_session_id == 'ses_x' + end) + + assert.are.equal('busy', loading_animation._animation.status_data.type) + end) end) - it('counts down retry seconds dynamically', function() - loading_animation._animation.status_data = { - type = 'retry', - message = 'Provider is overloaded', - attempt = 2, - next = 1018000, - } - - os.time = function() - return 1000 - end - local first = loading_animation._get_display_text() - - os.time = function() - return 1005 - end - local second = loading_animation._get_display_text() - - assert.is_truthy(first:find('in 18s', 1, true)) - assert.is_truthy(second:find('in 13s', 1, true)) + describe('sync_from_server (cache merge + replay)', function() + it('merges the response into the cache (only fills missing entries)', function() + state.jobs.set_api_client({ + list_session_status = function() + local p = require('opencode.promise').new() + p:resolve({ ses_x = { type = 'busy' } }) + return p + end, + }) + + loading_animation._animation.last_status_map.ses_x = { type = 'idle' } -- SSE won + loading_animation._animation.last_status_map.ses_y = { type = 'busy' } -- already cached + + loading_animation.sync_from_server() + vim.wait(200, function() + return false + end) + + assert.are.equal('idle', loading_animation._animation.last_status_map.ses_x.type) -- SSE preserved + assert.are.equal('busy', loading_animation._animation.last_status_map.ses_y.type) + end) + + it('replays only the active session after sync', function() + state.session.set_active({ id = 'ses_a' }) + state.jobs.set_api_client({ + list_session_status = function() + local p = require('opencode.promise').new() + p:resolve({ ses_a = { type = 'busy' }, ses_b = { type = 'busy' } }) + return p + end, + }) + + loading_animation.sync_from_server() + vim.wait(200, function() + return loading_animation._animation.status_data ~= nil + end) + + assert.are.equal('ses_a', loading_animation._animation.status_session_id) + assert.are.equal('busy', loading_animation._animation.last_status_map.ses_a.type) + assert.are.equal('busy', loading_animation._animation.last_status_map.ses_b.type) + end) + + it('does not regress to busy when sync returns a stale snapshot after SSE idle', function() + -- SSE already updated cache and status_data to idle for the active + -- session. sync's GET response arrives late with a stale busy + -- snapshot. The replay must not overwrite the fresher SSE state. + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + loading_animation._animation.last_status_map.ses_a = { type = 'idle' } + loading_animation._animation.status_data = { type = 'idle' } + loading_animation._animation.status_session_id = 'ses_a' + + state.jobs.set_api_client({ + list_session_status = function() + local p = require('opencode.promise').new() + p:resolve({ ses_a = { type = 'busy' } }) -- stale + return p + end, + }) + + loading_animation.sync_from_server() + vim.wait(200, function() + return false + end) + + assert.are.equal('idle', loading_animation._animation.status_data.type) + assert.is_false(loading_animation.is_running()) + end) end) - it('ignores status updates for non-active sessions', function() - state.session.set_active({ id = 'ses_active' }) - loading_animation._animation.status_data = nil + describe('setup / teardown', function() + it('hydrates via sync on setup, even when SSE has not seen this session yet', function() + state.session.set_active({ id = 'ses_x' }) + state.jobs.set_api_client({ + list_session_status = function() + local p = require('opencode.promise').new() + p:resolve({ ses_x = { type = 'busy' } }) + return p + end, + }) + + loading_animation.setup() + vim.wait(200, function() + return loading_animation._animation.status_data ~= nil + end) + + assert.are.equal('busy', loading_animation._animation.status_data.type) + end) + + it('clears all state on teardown so stale data does not survive a hide', function() + state.session.set_active({ id = 'ses_a' }) + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + loading_animation._animation.last_status_map.ses_a = { type = 'busy' } + + loading_animation.teardown() + + assert.is_nil(loading_animation._animation.status_data) + assert.is_nil(loading_animation._animation.status_session_id) + assert.is_nil(loading_animation._animation.timer) + assert.are.same({}, loading_animation._animation.last_status_map) + end) + + it('does not leave a stale spinner running when the model finishes during a hide', function() + state.session.set_active({ id = 'ses_a' }) + state.store.set_raw('windows', { output_buf = 1, footer_buf = 1 }) + loading_animation._animation.last_status_map.ses_a = { type = 'busy' } + loading_animation._animation.status_data = { type = 'busy' } + loading_animation._animation.status_session_id = 'ses_a' + + loading_animation.teardown() + -- ...the model finishes while the footer is hidden, the SSE + -- event goes nowhere... + + state.jobs.set_api_client({ + list_session_status = function() + local p = require('opencode.promise').new() + p:resolve({ ses_a = { type = 'idle' } }) + return p + end, + }) - loading_animation.on_session_status({ - sessionID = 'ses_other', - status = { type = 'retry', message = 'Provider is overloaded' }, - }) + loading_animation.setup() + vim.wait(200, function() + return loading_animation._animation.status_data ~= nil + and loading_animation._animation.status_data.type == 'idle' + end) - assert.is_nil(loading_animation._animation.status_data) + assert.are.equal('idle', loading_animation._animation.status_data.type) + assert.is_false(loading_animation.is_running()) + end) end) end)